Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate ssl common name #7114

Merged
merged 1 commit into from
Aug 17, 2022
Merged

Deprecate ssl common name #7114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changes/next-release/enhancement-Endpoints-23278.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"type": "enhancement",
"category": "Endpoints",
"description": "Enforce SSL common name as default endpoint url"
}
2 changes: 1 addition & 1 deletion awscli/clidriver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,7 @@ def __call__(self, args, parsed_globals):
event = 'before-building-argument-table-parser.%s.%s' % \
(self._parent_name, self._name)
self._emit(event, argument_table=self.arg_table, args=args,
session=self._session)
session=self._session, parsed_globals=parsed_globals)
operation_parser = self._create_operation_parser(self.arg_table)
self._add_help(operation_parser)
parsed_args, remaining = operation_parser.parse_known_args(args)
Expand Down
2 changes: 1 addition & 1 deletion awscli/customizations/commands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. 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.
  3. 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.

Copy link
Contributor Author

@dlm6693 dlm6693 Aug 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

session=self._session, parsed_globals=parsed_globals)
parser = ArgTableArgParser(self.arg_table, self.subcommand_table)
parsed_args, remaining = parser.parse_known_args(args)

Expand Down
138 changes: 138 additions & 0 deletions awscli/customizations/overridesslcommonname.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
# Copyright 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License"). You
# may not use this file except in compliance with the License. A copy of
# the License is located at
#
# http://aws.amazon.com/apache2.0/
#
# or in the "license" file accompanying this file. This file is
# distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
# ANY KIND, either express or implied. See the License for the specific
# language governing permissions and limitations under the License.


SSL_COMMON_NAMES = {
"sqs": {
"af-south-1": "af-south-1.queue.amazonaws.com",
"ap-east-1": "ap-east-1.queue.amazonaws.com",
"ap-northeast-1": "ap-northeast-1.queue.amazonaws.com",
"ap-northeast-2": "ap-northeast-2.queue.amazonaws.com",
"ap-northeast-3": "ap-northeast-3.queue.amazonaws.com",
"ap-south-1": "ap-south-1.queue.amazonaws.com",
"ap-southeast-1": "ap-southeast-1.queue.amazonaws.com",
"ap-southeast-2": "ap-southeast-2.queue.amazonaws.com",
"ap-southeast-3": "ap-southeast-3.queue.amazonaws.com",
"ca-central-1": "ca-central-1.queue.amazonaws.com",
"eu-central-1": "eu-central-1.queue.amazonaws.com",
"eu-north-1": "eu-north-1.queue.amazonaws.com",
"eu-south-1": "eu-south-1.queue.amazonaws.com",
"eu-west-1": "eu-west-1.queue.amazonaws.com",
"eu-west-2": "eu-west-2.queue.amazonaws.com",
"eu-west-3": "eu-west-3.queue.amazonaws.com",
"me-south-1": "me-south-1.queue.amazonaws.com",
"sa-east-1": "sa-east-1.queue.amazonaws.com",
"us-east-1": "queue.amazonaws.com",
"us-east-2": "us-east-2.queue.amazonaws.com",
"us-west-1": "us-west-1.queue.amazonaws.com",
"us-west-2": "us-west-2.queue.amazonaws.com",
"cn-north-1": "cn-north-1.queue.amazonaws.com.cn",
"cn-northwest-1": "cn-northwest-1.queue.amazonaws.com.cn",
"us-gov-west-1": "us-gov-west-1.queue.amazonaws.com",
"us-isob-east-1": "us-isob-east-1.queue.sc2s.sgov.gov",
},
"emr": {
"af-south-1": "af-south-1.elasticmapreduce.amazonaws.com",
"ap-east-1": "ap-east-1.elasticmapreduce.amazonaws.com",
"ap-northeast-1": "ap-northeast-1.elasticmapreduce.amazonaws.com",
"ap-northeast-2": "ap-northeast-2.elasticmapreduce.amazonaws.com",
"ap-northeast-3": "ap-northeast-3.elasticmapreduce.amazonaws.com",
"ap-south-1": "ap-south-1.elasticmapreduce.amazonaws.com",
"ap-southeast-1": "ap-southeast-1.elasticmapreduce.amazonaws.com",
"ap-southeast-2": "ap-southeast-2.elasticmapreduce.amazonaws.com",
"ap-southeast-3": "ap-southeast-3.elasticmapreduce.amazonaws.com",
"ca-central-1": "ca-central-1.elasticmapreduce.amazonaws.com",
"eu-north-1": "eu-north-1.elasticmapreduce.amazonaws.com",
"eu-south-1": "eu-south-1.elasticmapreduce.amazonaws.com",
"eu-west-1": "eu-west-1.elasticmapreduce.amazonaws.com",
"eu-west-2": "eu-west-2.elasticmapreduce.amazonaws.com",
"eu-west-3": "eu-west-3.elasticmapreduce.amazonaws.com",
"me-south-1": "me-south-1.elasticmapreduce.amazonaws.com",
"sa-east-1": "sa-east-1.elasticmapreduce.amazonaws.com",
"us-east-2": "us-east-2.elasticmapreduce.amazonaws.com",
"us-west-1": "us-west-1.elasticmapreduce.amazonaws.com",
"us-west-2": "us-west-2.elasticmapreduce.amazonaws.com",
},
"rds": {
"us-east-1": "rds.amazonaws.com",
},
"docdb": {
"us-east-1": "rds.amazonaws.com",
},
"neptune": {
"us-east-1": "rds.amazonaws.com",
},
"health": {
"aws-global": "health.us-east-1.amazonaws.com",
"af-south-1": "health.us-east-1.amazonaws.com",
"ap-east-1": "health.us-east-1.amazonaws.com",
"ap-northeast-1": "health.us-east-1.amazonaws.com",
"ap-northeast-2": "health.us-east-1.amazonaws.com",
"ap-northeast-3": "health.us-east-1.amazonaws.com",
"ap-south-1": "health.us-east-1.amazonaws.com",
"ap-southeast-1": "health.us-east-1.amazonaws.com",
"ap-southeast-2": "health.us-east-1.amazonaws.com",
"ap-southeast-3": "health.us-east-1.amazonaws.com",
"ca-central-1": "health.us-east-1.amazonaws.com",
"eu-central-1": "health.us-east-1.amazonaws.com",
"eu-north-1": "health.us-east-1.amazonaws.com",
"eu-south-1": "health.us-east-1.amazonaws.com",
"eu-west-1": "health.us-east-1.amazonaws.com",
"eu-west-2": "health.us-east-1.amazonaws.com",
"eu-west-3": "health.us-east-1.amazonaws.com",
"me-south-1": "health.us-east-1.amazonaws.com",
"sa-east-1": "health.us-east-1.amazonaws.com",
"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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Call the session.get_partition_for_region() for the provided region to determine the partition we are working in.
  2. 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)
  3. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"cn-north-1": "health.cn-northwest-1.amazonaws.com.cn",
"cn-northwest-1": "health.cn-northwest-1.amazonaws.com.cn",
"aws-cn-global": "health.cn-northwest-1.amazonaws.com.cn",
},
}

REGION_TO_PARTITION_OVERRIDE = {
"aws-global": "aws",
"aws-cn-global": "aws-cn",
}


def register_override_ssl_common_name(cli):
cli.register_last(
"before-building-argument-table-parser", update_endpoint_url
)


def update_endpoint_url(session, parsed_globals, **kwargs):
service = parsed_globals.command
endpoints = SSL_COMMON_NAMES.get(service)
region = session.get_config_variable("region")
# only change url if user has not overridden already themselves
if endpoints is not None and parsed_globals.endpoint_url is None:
endpoint_url = endpoints.get(region)
if endpoint_url is not None:
parsed_globals.endpoint_url = f"https://{endpoint_url}"
if service == "health":
_override_health_region(region, session, parsed_globals)


def _override_health_region(region, session, parsed_globals):
if region in REGION_TO_PARTITION_OVERRIDE:
partition = REGION_TO_PARTITION_OVERRIDE[region]
else:
partition = session.get_partition_for_region(region)
if partition == "aws-cn":
parsed_globals.region = "cn-northwest-1"
else:
parsed_globals.region = "us-east-1"
6 changes: 4 additions & 2 deletions awscli/customizations/rds.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,10 @@ class GenerateDBAuthTokenCommand(BasicCommand):

def _run_main(self, parsed_args, parsed_globals):
rds = self._session.create_client(
'rds', parsed_globals.region, parsed_globals.endpoint_url,
parsed_globals.verify_ssl
'rds',
region_name=parsed_globals.region,
endpoint_url=parsed_globals.endpoint_url,
verify=parsed_globals.verify_ssl
)
token = rds.generate_db_auth_token(
DBHostname=parsed_args.hostname,
Expand Down
2 changes: 2 additions & 0 deletions awscli/handlers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@
from awscli.customizations.sessionmanager import register_ssm_session
from awscli.customizations.sms_voice import register_sms_voice_hide
from awscli.customizations.dynamodb import register_dynamodb_paginator_fix
from awscli.customizations.overridesslcommonname import register_override_ssl_common_name


def awscli_initialize(event_handlers):
Expand Down Expand Up @@ -183,3 +184,4 @@ def awscli_initialize(event_handlers):
register_ssm_session(event_handlers)
register_sms_voice_hide(event_handlers)
register_dynamodb_paginator_fix(event_handlers)
register_override_ssl_common_name(event_handlers)
64 changes: 64 additions & 0 deletions tests/functional/test_override_ssl_common_name.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
# Copyright 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License"). You
# may not use this file except in compliance with the License. A copy of
# the License is located at
#
# http://aws.amazon.com/apache2.0/
#
# or in the "license" file accompanying this file. This file is
# distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
# ANY KIND, either express or implied. See the License for the specific
# language governing permissions and limitations under the License.

from awscli.testutils import BaseAWSCommandParamsTest
from awscli.customizations.overridesslcommonname import SSL_COMMON_NAMES


class OverrideSSLCommonNameTestCase(BaseAWSCommandParamsTest):
def _common_name_test_cases(self):

service_ops = {
"sqs": "list-queues",
"emr": "list-clusters",
"rds": "describe-db-clusters",
"neptune": "describe-db-clusters",
"docdb": "describe-db-clusters",
}
for service, operation in service_ops.items():
for region in SSL_COMMON_NAMES[service]:
yield (service, operation, region)

def _assert_region_endpoint_used(
self, expected_region, expected_endpoint_url
):
self.assertEqual(
expected_region, self.last_request_dict["context"]["client_region"]
)
self.assertEqual(
expected_endpoint_url,
self.last_request_dict["url"],
)

def test_set_endpoint_url_arg(self):
for service, operation, region in self._common_name_test_cases():
self.run_cmd(f"{service} {operation} --region {region}".split())
expected_endpoint_url = (
f"https://{SSL_COMMON_NAMES[service][region]}/"
)
self._assert_region_endpoint_used(region, expected_endpoint_url)

def test_override_health_endpoint_and_region(self):
expected_override_region = "us-east-1"
health_regions = SSL_COMMON_NAMES["health"]
for region in health_regions:
if region in ["cn-north-1", "cn-northwest-1", "aws-cn-global"]:
expected_override_region = "cn-northwest-1"
expected_endpoint_url = (
f"https://{SSL_COMMON_NAMES['health'][region]}/"
)
self.run_cmd(f"health describe-events --region {region}".split())
self._assert_region_endpoint_used(
expected_override_region,
expected_endpoint_url,
)
75 changes: 75 additions & 0 deletions tests/unit/customizations/test_overridesslcommonname.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
# Copyright 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License"). You
# may not use this file except in compliance with the License. A copy of
# the License is located at
#
# http://aws.amazon.com/apache2.0/
#
# or in the "license" file accompanying this file. This file is
# distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
# ANY KIND, either express or implied. See the License for the specific
# language governing permissions and limitations under the License.

from awscli.testutils import create_clidriver
from awscli.customizations.overridesslcommonname import (
update_endpoint_url,
SSL_COMMON_NAMES,
)

import pytest
import argparse


def parameters():
for service, regions in SSL_COMMON_NAMES.items():
for region in regions:
yield (service, region)


@pytest.fixture
def parsed_globals():
pg = argparse.Namespace()
pg.endpoint_url = None
pg.region = None
return pg


@pytest.fixture
def session():
driver = create_clidriver()
return driver.session


@pytest.mark.parametrize("service,region", parameters())
def test_update_endpoint_url(parsed_globals, session, service, region):
parsed_globals.command = service
session.set_config_variable("region", region)
update_endpoint_url(session, parsed_globals)
assert parsed_globals.endpoint_url == (
f"https://{SSL_COMMON_NAMES[service][region]}"
)


@pytest.mark.parametrize("service,region", parameters())
def test_url_modified_from_event(parsed_globals, session, service, region):
assert parsed_globals.endpoint_url is None
parsed_globals.command = service
session.set_config_variable("region", region)
session.emit(
f"before-building-argument-table-parser.{service}",
args=[],
session=session,
argument_table={},
parsed_globals=parsed_globals,
)
assert parsed_globals.endpoint_url == (
f"https://{SSL_COMMON_NAMES[service][region]}"
)


def test_dont_modify_provided_url(parsed_globals, session):
parsed_globals.endpoint_url = "http://test.com"
parsed_globals.command = "sqs"
update_endpoint_url(session, parsed_globals)
assert parsed_globals.endpoint_url == "http://test.com"