Deprecate ssl common name #7114

dlm6693 · 2022-07-15T19:09:57Z

This is an attempt to maintain the usage of sslCommonName in the AWS CLI. A handler is added that adds the endpoint-url arg to any given command if the user has not provided it themselves and the service and region are a match from a provided list.

It will be deprecated in botocore but cannot in the CLI as it causes breaking changes. This PR will counteract that. All SSL common name endpoints are hardcoded

awscli/clidriver.py

awscli/customizations/commands.py

tests/unit/customizations/test_overridesslcommonname.py

awscli/customizations/overridesslcommonname.py

tests/unit/customizations/test_overridesslcommonname.py

nateprewitt

Can we take a closer look at the endpoints entries? It looks like we're creating unroutable URIs with this change.

awscli/customizations/overridesslcommonname.py

tests/unit/customizations/test_overridesslcommonname.py

codecov-commenter · 2022-07-19T17:54:45Z

Codecov Report

Merging #7114 (efa3351) into develop (1dbfc11) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #7114   +/-   ##
========================================
  Coverage    92.88%   92.89%           
========================================
  Files          204      205    +1     
  Lines        16351    16374   +23     
========================================
+ Hits         15188    15211   +23     
  Misses        1163     1163

Impacted Files	Coverage Δ
awscli/clidriver.py	`95.97% <ø> (ø)`
awscli/customizations/commands.py	`99.56% <ø> (ø)`
awscli/customizations/rds.py	`100.00% <ø> (ø)`
awscli/customizations/overridesslcommonname.py	`100.00% <100.00%> (ø)`
awscli/handlers.py	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

awscli/customizations/overridesslcommonname.py

awscli/testutils.py

tests/functional/test_override_ssl_common_name.py

tests/integration/customizations/test_override_ssl_common_name.py

tests/unit/customizations/test_overridesslcommonname.py

awscli/customizations/rds.py

nateprewitt

Looks good to me ⛵

kyleknap

Looks good. I just had some of suggestions and questions

awscli/customizations/overridesslcommonname.py

tests/functional/test_override_ssl_common_name.py

tests/unit/customizations/test_overridesslcommonname.py

kyleknap · 2022-08-04T00:39:38Z

awscli/customizations/commands.py

@@ -134,7 +134,7 @@ def __call__(self, args, parsed_globals):
        event = 'before-building-argument-table-parser.%s' % \
            ".".join(self.lineage_names)
        self._session.emit(event, argument_table=self._arg_table, args=args,
-                           session=self._session)


Could you elaborate on the raitonale on why before-building-argument-table-parser was chosen for this handler? I think the main concerns right now are:

We are adding a new keyword argument (parsed_globals) to the event. As a rule of thumb, new arguments are added if they make sense and are needed.

It is possible that this event can be emitted twice. I was seeing this with the aws rds wait db-cluster-available command. Fortunately it looks like the handler is idempotent but it would be nice if we did not have to rely on that.

The purpose of the before-building-argument-table-parser is for updating the argument table prior to parsing and I'm not sure it logically fits with setting values on the parsed globals.

I have not come up with a good alternative yet, but I just wanted to put some initial thoughts down.

I know we had originally agreed upon building-command-table, but upon diving into the code, this seemed by far to be the less invasive approach. I do see now that it runs twice, once for the service and then for the operation, which is definitely behavior that should be avoided even if the handler is idempotent.

The trouble is figuring out a clean implementation of passing parsed_globals to that event. The ServiceOperation object only gets that variable when its invoked in a ServiceCommand object's __call__ method, which is after building-command-table has run. I think its definitely doable, but would require a larger refactor, which I wanted to avoid for something that is pretty core behavior to the CLI.

The easiest way that is occurring to me now would be to pass parsed_globals to _get_command_table, which would pass it in turn to _create_command_table. Then it could be added to the event. Let me know what you think of that approach and if you have any concerns with it. Happy to investigate further, but if you do come up with any suggestions let me know.

I think originally I had two different potential events in mind:

top-level-args-parsed - This is the event for when the global args get parsed. It seemed suiting as we would just immediately set the global argument of --endpoint-url at that stage and we can keep endpoint url mutations isolated to that stage. The only issue with that approach is that the session had technically not been initialized yet, so there may be unintended consequences/configuration resolution if we try to grab the region before that. This event also happens before even logging is enabled so that is a concern as well if we are trying to do any tracing of it.

session-initialized - This seemed like the next best event to use as the session was initialized there. The only problem I saw in using this event is that we could not 100% capture correctly the service being used. Specifically, for CLI aliases the command that gets parsed is represented as the alias and not the underlying service command at that point. So we would be using the wrong endpoint if the command was under an alias.

This all being said. I don't know if I have a clear preference between building-command-table and before-building-argument-table-parser. They both share similar concerns, but it seems like we need an event lower into the stack to get this to work appropriately and I do not want to be adding a new event for this. I think for now let's just stick with before-building-argument-table-parser unless we get a better idea.

Ok cool sounds good Kyle. I'll keep the behavior as is. I'll be adding the additional services that were missed in my script that you caught in yours shortly.

tests/functional/test_override_ssl_common_name.py

kyleknap

It's looking good. I mainly focused on looking through the endpoint mapping to make sure they were valid.

tests/unit/customizations/test_overridesslcommonname.py

awscli/customizations/overridesslcommonname.py

kyleknap · 2022-08-10T17:00:01Z

awscli/customizations/overridesslcommonname.py

+        "us-east-1": "health.us-east-1.amazonaws.com",
+        "us-east-2": "health.us-east-1.amazonaws.com",
+        "us-west-1": "health.us-east-1.amazonaws.com",
+        "us-west-2": "health.us-east-1.amazonaws.com",


So when running the CLI manually with these hard coded endpoints, I'm getting the following error for non-us-east-1 regions when using health:

$ aws health describe-events --region us-west-2 An error occurred (InvalidSignatureException) when calling the DescribeEvents operation: Credential should be scoped to a valid region.

It looks like the reason this is happening is that health is defined as a non-regionalized service and thus has a default credentialScope of us-east-1 for the aws partition. We are going to have to adjust the logic here so that we also set the region to the default credential scope for health as well as the endpoint url. I would not worry about generalizing it too much. It should be just:

If we find we need to set the endpoint url for health:

Call the session.get_partition_for_region() for the provided region to determine the partition we are working in.

Map the partition to the value representing the region of the default credential scope for the partition (i.e. determine if the region we need to set should be us-east-1, cn-northwest-1)

Set the region either on the parsed_globals or via the session.set_config_variable() method, whatever does the job of making sure we use the correct credential scope for the region.

alright i shot up a fix. i was getting the same error before making the fix, but a different one. not sure how to handle it since it was a subscription error but as far as i can tell there are no subscriptions in aws health.

awscli/customizations/overridesslcommonname.py

kyleknap

Looks great! Can you squash all of these commits into a single commit? After, I'll go ahead and approve and merge the PR.

kyleknap

Thanks! 🚢

stealthycoin

The code looks a lot better now, I think the one main thing I want to se updated before we merge is the commit title/message. This is a fairly involved change and we should document some of the conversations we had in here in the commit.

Think of someone trying to figure out why the code does what it does, and historically why we chose to do what we did. For example if someone was looking at the session.emit line from here and wondering why we added parsed_globals as an argument to that event and they check the git blame it just says "deprecate ssl common name" which is not helpful. You don't need to copy and paste all of the conversations into the commit, but I would like to see a few paragraphs outlining some of the important design considerations and consequences of this change.

stealthycoin

A changelog entry would be nice as well. I think this change is a little too involved to be omitted even though it shouldn't have any consequences.

dlm6693 · 2022-08-16T22:00:09Z

@stealthycoin sure thing. i'll have something ready in a bit.

kyleknap

Just had a couple more comments after the recent updates.

kyleknap · 2022-08-17T16:14:31Z

auditendpoints.py

@@ -0,0 +1,37 @@
+import json


I'm not sure if this file should have been added to the PR.

Oops! I thought this was in my global git ignore but I guess it snuck through. Will remove

kyleknap · 2022-08-17T16:22:54Z

.changes/next-release/enhancement-Endpoints-23278.json

+{
+  "type": "enhancement",
+  "category": "Endpoints",
+  "description": "Allow usage of SSL common name with deprecation"


For the description, let's update it to: "Enforce SSL common name as default endpoint url". Mainly the wording is a bit more consistent with the change we are making as "Allow usage" is more indicative of adding an opt-in option or loosening a constraint.

Got it! Will update.

In the comming months, the method by which every AWS SDK resolves their endpoints, including aws-cli v1 and boto3, will be udpated with completely different behavior. At this time,the use of the `sslCommonName` paradigm will be fully deprecated. Additionally, it was mainly used to support python 2.6, which has not been supported by boto3 or the CLI since January 1, 2020. The boto team wanted to ease the transition by raising a warning to users and providing an optional environment variable to fully disable the behavior immediately. However, this causes breaking changes in v1 of aws-cli so additional changes were required in it to release the deprecation. This commit attempts to formally maintain the `sslCommonName` paradigm in aws-cli v1. This is accomplished by providing hardcoded, manual overrides for each service and region combination where this parameter is used over the standard `hostname`. The list of these service/region combinations was compiled by creating a client for every single combination and emitting the `endpoint_url` to a file twice. The first using a version of botocore that uses `sslCommonName` and one that doesn't, then diffing the two files to see which URLs changed. While behavior for the end user will largely be the same, if they do not disable the common name behavior by setting the environment variable `BOTO_DISABLE_COMMONNAME` to `true`, a warning will be omitted in the console once per session. Additional details can be found in boto/botocore#2705. The override occurs when the `before-building-argument-table-parser` is omitted by an event handler. At this point, if the user has not set `endpoint_url` in `parsed_globals` (`--endpoint-url` flag from the command line) the endpoint that the CLI uses will be overridden to continue to use the SSL common name. There were a few different options of which event to run the override in, but ultimately this one was chosen because it was the least invasive to the existing code base. As this is very low level behavior for the CLI, minimizing the amount of changes made was prioritized over one that might seemt to make more logical sense like `top-level-args-parsed`, which is when the arguments passed to the command line are actually parsed through. However, the usage of a session instance was required in order to get config variables like region and partition. There is a strong possiblity that this customization override will run twice per call, but that is a design flaw the maintainers are aware of and have accpeted. Additionally for the `health` service, the region parameter needed to be overwritten because of it's use of pseudo regions like `aws-global`. In the other cases, the region within the URL essentially always matched the region, but since health uses the SSL common name in global pseudo regions, this change was required as well or an exception would be raised.

* release-1.25.54: Bumping version to 1.25.54 Update changelog based on model updates Allow usage of SSL common name with deprecation. (#7114) Update get-secret-value.rst New CLI examples ecs, iotsitewise, ivs, appmesh, memorydb Update examples for secretsmanager

This reverts commit eb696a8.

* release-1.25.55: Bumping version to 1.25.55 Add changelog for revert Revert "Allow usage of SSL common name with deprecation. (#7114)"

In the comming months, the method by which every AWS SDK resolves their endpoints, including aws-cli v1 and boto3, will be udpated with completely different behavior. At this time,the use of the `sslCommonName` paradigm will be fully deprecated. Additionally, it was mainly used to support python 2.6, which has not been supported by boto3 or the CLI since January 1, 2020. The boto team wanted to ease the transition by raising a warning to users and providing an optional environment variable to fully disable the behavior immediately. However, this causes breaking changes in v1 of aws-cli so additional changes were required in it to release the deprecation. This commit attempts to formally maintain the `sslCommonName` paradigm in aws-cli v1. This is accomplished by providing hardcoded, manual overrides for each service and region combination where this parameter is used over the standard `hostname`. The list of these service/region combinations was compiled by creating a client for every single combination and emitting the `endpoint_url` to a file twice. The first using a version of botocore that uses `sslCommonName` and one that doesn't, then diffing the two files to see which URLs changed. While behavior for the end user will largely be the same, if they do not disable the common name behavior by setting the environment variable `BOTO_DISABLE_COMMONNAME` to `true`, a warning will be omitted in the console once per session. Additional details can be found in boto/botocore#2705. The override occurs when the `before-building-argument-table-parser` is omitted by an event handler. At this point, if the user has not set `endpoint_url` in `parsed_globals` (`--endpoint-url` flag from the command line) the endpoint that the CLI uses will be overridden to continue to use the SSL common name. There were a few different options of which event to run the override in, but ultimately this one was chosen because it was the least invasive to the existing code base. As this is very low level behavior for the CLI, minimizing the amount of changes made was prioritized over one that might seemt to make more logical sense like `top-level-args-parsed`, which is when the arguments passed to the command line are actually parsed through. However, the usage of a session instance was required in order to get config variables like region and partition. There is a strong possiblity that this customization override will run twice per call, but that is a design flaw the maintainers are aware of and have accpeted. Additionally for the `health` service, the region parameter needed to be overwritten because of it's use of pseudo regions like `aws-global`. In the other cases, the region within the URL essentially always matched the region, but since health uses the SSL common name in global pseudo regions, this change was required as well or an exception would be raised.

This reverts commit eb696a8.

In the comming months, the method by which every AWS SDK resolves their endpoints, including aws-cli v1 and boto3, will be udpated with completely different behavior. At this time,the use of the `sslCommonName` paradigm will be fully deprecated. Additionally, it was mainly used to support python 2.6, which has not been supported by boto3 or the CLI since January 1, 2020. The boto team wanted to ease the transition by raising a warning to users and providing an optional environment variable to fully disable the behavior immediately. However, this causes breaking changes in v1 of aws-cli so additional changes were required in it to release the deprecation. This commit attempts to formally maintain the `sslCommonName` paradigm in aws-cli v1. This is accomplished by providing hardcoded, manual overrides for each service and region combination where this parameter is used over the standard `hostname`. The list of these service/region combinations was compiled by creating a client for every single combination and emitting the `endpoint_url` to a file twice. The first using a version of botocore that uses `sslCommonName` and one that doesn't, then diffing the two files to see which URLs changed. While behavior for the end user will largely be the same, if they do not disable the common name behavior by setting the environment variable `BOTO_DISABLE_COMMONNAME` to `true`, a warning will be omitted in the console once per session. Additional details can be found in boto/botocore#2705. The override occurs when the `before-building-argument-table-parser` is omitted by an event handler. At this point, if the user has not set `endpoint_url` in `parsed_globals` (`--endpoint-url` flag from the command line) the endpoint that the CLI uses will be overridden to continue to use the SSL common name. There were a few different options of which event to run the override in, but ultimately this one was chosen because it was the least invasive to the existing code base. As this is very low level behavior for the CLI, minimizing the amount of changes made was prioritized over one that might seemt to make more logical sense like `top-level-args-parsed`, which is when the arguments passed to the command line are actually parsed through. However, the usage of a session instance was required in order to get config variables like region and partition. There is a strong possiblity that this customization override will run twice per call, but that is a design flaw the maintainers are aware of and have accpeted. Additionally for the `health` service, the region parameter needed to be overwritten because of it's use of pseudo regions like `aws-global`. In the other cases, the region within the URL essentially always matched the region, but since health uses the SSL common name in global pseudo regions, this change was required as well or an exception would be raised.

This reverts commit eb696a8.

In the comming months, the method by which every AWS SDK resolves their endpoints, including aws-cli v1 and boto3, will be udpated with completely different behavior. At this time,the use of the `sslCommonName` paradigm will be fully deprecated. Additionally, it was mainly used to support python 2.6, which has not been supported by boto3 or the CLI since January 1, 2020. The boto team wanted to ease the transition by raising a warning to users and providing an optional environment variable to fully disable the behavior immediately. However, this causes breaking changes in v1 of aws-cli so additional changes were required in it to release the deprecation. This commit attempts to formally maintain the `sslCommonName` paradigm in aws-cli v1. This is accomplished by providing hardcoded, manual overrides for each service and region combination where this parameter is used over the standard `hostname`. The list of these service/region combinations was compiled by creating a client for every single combination and emitting the `endpoint_url` to a file twice. The first using a version of botocore that uses `sslCommonName` and one that doesn't, then diffing the two files to see which URLs changed. While behavior for the end user will largely be the same, if they do not disable the common name behavior by setting the environment variable `BOTO_DISABLE_COMMONNAME` to `true`, a warning will be omitted in the console once per session. Additional details can be found in boto/botocore#2705. The override occurs when the `before-building-argument-table-parser` is omitted by an event handler. At this point, if the user has not set `endpoint_url` in `parsed_globals` (`--endpoint-url` flag from the command line) the endpoint that the CLI uses will be overridden to continue to use the SSL common name. There were a few different options of which event to run the override in, but ultimately this one was chosen because it was the least invasive to the existing code base. As this is very low level behavior for the CLI, minimizing the amount of changes made was prioritized over one that might seemt to make more logical sense like `top-level-args-parsed`, which is when the arguments passed to the command line are actually parsed through. However, the usage of a session instance was required in order to get config variables like region and partition. There is a strong possiblity that this customization override will run twice per call, but that is a design flaw the maintainers are aware of and have accpeted. Additionally for the `health` service, the region parameter needed to be overwritten because of it's use of pseudo regions like `aws-global`. In the other cases, the region within the URL essentially always matched the region, but since health uses the SSL common name in global pseudo regions, this change was required as well or an exception would be raised.

This reverts commit eb696a8.

dlm6693 force-pushed the deprecate-ssl-common-name branch from bcea441 to c55af97 Compare July 15, 2022 19:13

dlm6693 requested review from kyleknap and nateprewitt July 15, 2022 19:15

dlm6693 commented Jul 18, 2022

View reviewed changes

awscli/clidriver.py Show resolved Hide resolved

dlm6693 commented Jul 18, 2022

View reviewed changes

awscli/customizations/commands.py Show resolved Hide resolved

dlm6693 requested a review from stealthycoin July 18, 2022 17:17

stealthycoin suggested changes Jul 18, 2022

View reviewed changes

nateprewitt requested changes Jul 19, 2022

View reviewed changes

dlm6693 force-pushed the deprecate-ssl-common-name branch from c55af97 to d0e5dc2 Compare July 19, 2022 17:53

nateprewitt requested changes Jul 19, 2022

View reviewed changes

dlm6693 commented Jul 20, 2022

View reviewed changes

awscli/customizations/rds.py Show resolved Hide resolved

dlm6693 force-pushed the deprecate-ssl-common-name branch 2 times, most recently from 1b54127 to f0c9e33 Compare July 20, 2022 17:16

nateprewitt approved these changes Jul 20, 2022

View reviewed changes

kyleknap reviewed Aug 4, 2022

View reviewed changes

dlm6693 force-pushed the deprecate-ssl-common-name branch from eb1af3b to 46eb4d4 Compare August 4, 2022 13:37

dlm6693 commented Aug 4, 2022

View reviewed changes

tests/functional/test_override_ssl_common_name.py Outdated Show resolved Hide resolved

kyleknap reviewed Aug 10, 2022

View reviewed changes

dlm6693 force-pushed the deprecate-ssl-common-name branch from 9509150 to a2a87c7 Compare August 10, 2022 21:37

kyleknap reviewed Aug 16, 2022

View reviewed changes

dlm6693 force-pushed the deprecate-ssl-common-name branch from 6fdd27f to efa3351 Compare August 16, 2022 19:35

kyleknap approved these changes Aug 16, 2022

View reviewed changes

stealthycoin suggested changes Aug 16, 2022

View reviewed changes

stealthycoin reviewed Aug 16, 2022

View reviewed changes

dlm6693 mentioned this pull request Aug 16, 2022

Deprecate ssl common name boto/botocore#2738

Merged

dlm6693 force-pushed the deprecate-ssl-common-name branch 3 times, most recently from 9175185 to 01d71e3 Compare August 17, 2022 14:11

kyleknap reviewed Aug 17, 2022

View reviewed changes

dlm6693 force-pushed the deprecate-ssl-common-name branch from 01d71e3 to ce2d62c Compare August 17, 2022 16:44

stealthycoin approved these changes Aug 17, 2022

View reviewed changes

kyleknap approved these changes Aug 17, 2022

View reviewed changes

dlm6693 merged commit eb696a8 into aws:develop Aug 17, 2022

dlm6693 deleted the deprecate-ssl-common-name branch August 17, 2022 17:16

kbehlers mentioned this pull request Aug 17, 2022

Cannot configure new profiles using aws configure --profile option in v1.25.54 #7199

Closed

kyleknap added a commit that referenced this pull request Aug 17, 2022

Revert "Allow usage of SSL common name with deprecation. (#7114)"

490b6f1

This reverts commit eb696a8.

kyleknap mentioned this pull request Aug 17, 2022

Revert "Deprecate ssl common name" #7200

Merged

aws-sdk-python-automation added a commit that referenced this pull request Aug 17, 2022

Merge branch 'release-1.25.55'

c2a8810

* release-1.25.55: Bumping version to 1.25.55 Add changelog for revert Revert "Allow usage of SSL common name with deprecation. (#7114)"

hssyoo pushed a commit to hssyoo/aws-cli that referenced this pull request Aug 18, 2022

Revert "Allow usage of SSL common name with deprecation. (aws#7114)"

faac90a

This reverts commit eb696a8.

ConnorKirk pushed a commit to ConnorKirk/aws-cli that referenced this pull request Oct 28, 2022

Revert "Allow usage of SSL common name with deprecation. (aws#7114)"

693c028

This reverts commit eb696a8.

godd9170 pushed a commit to godd9170/aws-cli that referenced this pull request Mar 7, 2023

Revert "Allow usage of SSL common name with deprecation. (aws#7114)"

601832f

This reverts commit eb696a8.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate ssl common name #7114

Deprecate ssl common name #7114

dlm6693 commented Jul 15, 2022 •

edited

Loading

nateprewitt left a comment

codecov-commenter commented Jul 19, 2022 •

edited

Loading

nateprewitt left a comment

kyleknap left a comment

kyleknap Aug 4, 2022

dlm6693 Aug 4, 2022 •

edited

Loading

kyleknap Aug 4, 2022

dlm6693 Aug 8, 2022

kyleknap left a comment

kyleknap Aug 10, 2022

dlm6693 Aug 10, 2022

kyleknap left a comment

kyleknap left a comment

stealthycoin left a comment

stealthycoin left a comment

dlm6693 commented Aug 16, 2022

kyleknap left a comment

kyleknap Aug 17, 2022

dlm6693 Aug 17, 2022

kyleknap Aug 17, 2022 •

edited

Loading

dlm6693 Aug 17, 2022

Deprecate ssl common name #7114

Deprecate ssl common name #7114

Conversation

dlm6693 commented Jul 15, 2022 • edited Loading

nateprewitt left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 19, 2022 • edited Loading

Codecov Report

nateprewitt left a comment

Choose a reason for hiding this comment

kyleknap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlm6693 Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyleknap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyleknap left a comment

Choose a reason for hiding this comment

kyleknap left a comment

Choose a reason for hiding this comment

stealthycoin left a comment

Choose a reason for hiding this comment

stealthycoin left a comment

Choose a reason for hiding this comment

dlm6693 commented Aug 16, 2022

kyleknap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyleknap Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlm6693 commented Jul 15, 2022 •

edited

Loading

codecov-commenter commented Jul 19, 2022 •

edited

Loading

dlm6693 Aug 4, 2022 •

edited

Loading

kyleknap Aug 17, 2022 •

edited

Loading