Add SSL support to local start-api and start-lambda #5902

mbklein · 2023-09-07T18:55:53Z

Which issue(s) does this change fix?

Why is this change necessary?

As a few people mentioned in #3193, our API allows users to authenticate against our institution's local single-sign-on service. It has proven impractical to try to do rapid development and debugging using sam sync. We have been working around this by using an SSL proxy running in Docker (first using nginx and then haproxy) to proxy connections from https:3002 to http:3000. Lately, even this workaround has started to introduce issues, as the proxies' own quirks have resulted in stripped response headers and failed rewrites.

How does it address the issue?

It adds two new parameters, --ssl-cert-file and --ssl-key-file, to sam local start-api and start-lambda. (It was my intention only to add this functionality to start-api, but the CLI options for both are already defined in common, so it seemed easier just to go with it.) By default, without those parameters specified, the command behaves as it does now. With both specified, their values are passed through as a tuple to the Flask app, so it can start with a defined SSL context.

What side effects does this change have?

None. Default behavior remains the same, and all new functionality is handled by Flask.

Mandatory Checklist

PRs will only be reviewed after checklist is complete

Add input/output type hints to new functions/methods
(N/A) Write design document if needed (Do I need to write a design document?)
- I don't believe this change requires a design document, as the change to the CLI is fully backward compatible
Write/update unit tests
(N/A) Write/update integration tests
- Other than generating a self-signed certificate at test time and issuing an HTTPS request to make sure Flask picks up the new option correctly, I don't know what new integration tests might be required. I'm open to pursuing that if required to consider this PR.
(N/A) Write/update functional tests if needed
make pr passes
(N/A) make update-reproducible-reqs if dependencies were changed
Write documentation
- Updated samcli.json to include new CLI parameter specs, which I assume is all that's needed

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

mndeveci · 2023-09-08T21:16:55Z

Thanks a lot for working on this change!

Since this involves certificates, it is better to confirm changes with security team. We will be booking some time with them and come back to you if they have any suggestions.

mndeveci

Thanks again for your contribution and you patience. We've got approval to move forward for this change, I left some comments in the PR.

mndeveci · 2023-11-21T00:53:19Z

samcli/commands/local/cli_common/options.py

+            click.option("--ssl-cert-file", default=None, help="Path to SSL certificate file (default: None)"),
+            click.option("--ssl-key-file", default=None, help="Path to SSL key file (default: None)"),


Does defining one of these parameter will require other to be present? If so we can add a validation for that.

Yes, both the certificate and and key must be specified in order for SSL to work. I'll see about adding that validation.

mndeveci · 2023-11-21T00:54:48Z

samcli/commands/local/start_api/cli.py

+        error_message = (
+            "SSL key file must be present if certificate is present"
+            if ssl_key_file is None
+            else "Invalid certificate and/or key file"
+        )


Is this the case all the time? Can we just re-surface the original error caught here?

If I add the “if either is present, both must be present” validation you suggested above, we can probably just remove this block entirely and let the SSLError bubble up on its own. I only added it to differentiate between a real “bad cert or key” error and a missing key.

My bad, I think I've read this line wrong at first, but now it makes more sense.

If you think SSLError text is good enough, we can bubble up the error or just its message back to the user. And once we are surfacing these errors, we need to make sure we are raising UserError so that CLI will handle them gracefully rather than crashing with exception trace (I can check that one later once you made the changes).

I think the SSLError text is good enough (and I'm not sure we can cover all possible SSL Errors with our own text anyway), but I'll still catch it and wrap it in a UserError. The click options validator will catch the other case of mutually inclusive options.

samcli/commands/local/lib/local_api_service.py

samcli/local/lambda_service/local_lambda_invoke_service.py

mbklein · 2023-11-28T00:31:27Z

I've added a validation callback to the CLI options spec to make sure that --ssl-cert-file and --ssl-key-file are both either present or missing, and that any value given for either of them is a valid file that exists. I also updated the SSLError handler to pass through the specific SSL error.

I see what you mean about using @dataclass, but since Flask.app.run() ultimately expects a (cert_file, key_file) tuple, it might make sense just to keep it that way? If you think a higher-level construct would make the code more readable, I'll do it, but it'll just have to be unwrapped back into a tuple further down the stack.

Come to think of it, Flask is using werkzeug under the hood, and it's werkzeug.serving.run_simple() that handles the SSL options. The value can be a (cert, key) tuple, a full ssl.SSLContext object, or the string 'adhoc' (to use a one-off self-signed certificate). In order to keep this both simple and relatively safe, I went with the tuple. If a higher-level class is preferable, I could just create an ssl.SSLContext here and pass that around instead.

mndeveci

One small comment form my side, rest is looking good, thanks again for the changes!

The value can be a (cert, key) tuple, a full ssl.SSLContext object, or the string 'adhoc' (to use a one-off self-signed certificate). In order to keep this both simple and relatively safe, I went with the tuple. If a higher-level class is preferable, I could just create an ssl.SSLContext here and pass that around instead.

Regarding to this ^, let's keep tuple instead, I don't want to include 3rd part object/type into this code base in case they will change its definition in the future.

mndeveci · 2023-12-01T18:33:58Z

samcli/commands/local/cli_common/options.py

+            click.option("--ssl-cert-file", default=None, callback=validate_ssl_params, help="Path to SSL certificate file (default: None)"),
+            # Mark --ssl-key-file as eager, so by the time the --ssl-cert-file validator is invoked, we know if it's missing
+            click.option("--ssl-key-file", default=None, callback=validate_ssl_params, is_eager=True, help="Path to SSL key file (default: None)"),


Sorry that I missed this in the first round but we could use Click's file path arguments which will provide some basic path validation out of the box. https://click.palletsprojects.com/en/8.1.x/arguments/#file-path-arguments

Nice catch, thanks. That does make things a lot simpler, and eliminates the need to call the custom validator on the first pass.

mndeveci

I've updated one output which was missing https if these parameters are set. Other than that it looks good to me now.

One final question, do you think this will be useful for sam local start-lambda? If not I think we can remove these parameters for that command and keep it only for sam local start-api for now.

mbklein · 2023-12-09T15:38:09Z

One final question, do you think this will be useful for sam local start-lambda?

I don't really know. I don't have a use case for it. The only reason I added it in the first place was because of the common set of kwargs that get passed down to both service classes. I'm not opposed to removing the command line options as long as you don't mind having those unused arguments hanging out on the lambda class.

mbklein · 2023-12-11T17:23:56Z

@mndeveci Would you like me to change this so that the parameters only apply to start-api?

mndeveci · 2023-12-11T18:02:46Z

@mndeveci Would you like me to change this so that the parameters only apply to start-api?

I can't find request to add this for start-lambda in the original issue as well. Would you mind removing that parameter from start-lambda?

lucashuy · 2023-12-11T19:36:46Z

samcli/commands/local/cli_common/options.py

+            # Mark --ssl-key-file as eager, so by the time the --ssl-cert-file validator is invoked,
+            # we know if it's missing


nit: Another option is to use the ClickMutex class we have to set these two options as required.

An example of this being implemented can be found here:

aws-sam-cli/samcli/commands/init/command.py

Lines 117 to 130 in 2a527fd

@click.option(

"--no-interactive",

is_flag=True,

default=False,

help="Disable interactive prompting for init parameters. (fail if any required values are missing)",

cls=ClickMutex,

required_param_lists=[

["name", "location"],

["name", "package_type", "base_image"],

["name", "runtime", "dependency_manager", "app_template"],

# check non_interactive_validation for additional validations

],

required_params_hint=REQUIRED_PARAMS_HINT,

)

if you had wanted to implement it this way.

Excellent, I was looking for something like that and didn't see it. I'll look into that in this last round. Thanks!

mbklein · 2023-12-12T16:07:26Z

@mndeveci I've removed the SSL options from the start-lambda command and replaced the clunky validate_ssl_params callback with the ClickMutex option @lucashuy suggested. I think that's everything. Thanks for taking the time to work with me on this.

lucashuy

Thanks for making the changes!

This reverts commit 49e4d70.

Add SSL support to local start-api and start-lambda

2853bb4

mbklein requested a review from a team as a code owner September 7, 2023 18:55

mbklein requested review from moelasmar and mildaniel September 7, 2023 18:55

Merge branch 'develop' into add-local-ssl-support

93a5cef

mndeveci added blocked/pending-security-review The PR cannot be merged or reviewed due to pending security review. and removed stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Oct 18, 2023

mndeveci requested changes Nov 21, 2023

View reviewed changes

mbklein added 2 commits November 27, 2023 23:02

Merge branch 'develop' into add-local-ssl-support

be55d7c

Validate SSL options earlier; simplify SSLError handler

788fcea

mbklein force-pushed the add-local-ssl-support branch from bc4ca00 to 788fcea Compare November 28, 2023 00:13

mndeveci requested changes Dec 1, 2023

View reviewed changes

Use click.Path(exists=True) for SSL file validation

d1e5d9a

mbklein requested a review from mndeveci December 1, 2023 21:37

mbklein and others added 2 commits December 2, 2023 11:48

Merge branch 'develop' into add-local-ssl-support

b52eb29

update tests & formatting

ce65366

mndeveci removed the blocked/pending-security-review The PR cannot be merged or reviewed due to pending security review. label Dec 9, 2023

mndeveci reviewed Dec 9, 2023

View reviewed changes

mbklein requested a review from mndeveci December 11, 2023 17:18

Merge branch 'develop' into add-local-ssl-support

42b9d9d

lucashuy reviewed Dec 11, 2023

View reviewed changes

Remove SSL options from start-lambda

4526c91

mndeveci approved these changes Dec 13, 2023

View reviewed changes

Merge branch 'develop' into add-local-ssl-support

f9f1fc6

mndeveci removed the request for review from moelasmar December 13, 2023 22:37

lucashuy approved these changes Dec 14, 2023

View reviewed changes

lucashuy added 2 commits December 14, 2023 10:27

Merge branch 'develop' into add-local-ssl-support

e6273bf

Merge branch 'develop' into add-local-ssl-support

fe37114

lucashuy enabled auto-merge December 14, 2023 19:47

lucashuy added this pull request to the merge queue Dec 14, 2023

Merged via the queue into aws:develop with commit 49e4d70 Dec 14, 2023
55 checks passed

lucashuy added a commit that referenced this pull request Dec 18, 2023

Revert "Add SSL support to local start-api and start-lambda (#5902)"

64b72a6

This reverts commit 49e4d70.

lucashuy mentioned this pull request Dec 18, 2023

Revert "Add SSL support to local start-api and start-lambda" #6467

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SSL support to local start-api and start-lambda #5902

Add SSL support to local start-api and start-lambda #5902

mbklein commented Sep 7, 2023

mndeveci commented Sep 8, 2023

mndeveci left a comment

mndeveci Nov 21, 2023

mbklein Nov 27, 2023

mndeveci Nov 21, 2023

mbklein Nov 27, 2023

mndeveci Nov 27, 2023

mbklein Nov 27, 2023

mbklein commented Nov 28, 2023

mndeveci left a comment

mndeveci Dec 1, 2023

mbklein Dec 1, 2023

mndeveci left a comment

mbklein commented Dec 9, 2023

mbklein commented Dec 11, 2023

mndeveci commented Dec 11, 2023

lucashuy Dec 11, 2023

mbklein Dec 11, 2023

mbklein commented Dec 12, 2023

lucashuy left a comment

		click.option("--ssl-cert-file", default=None, help="Path to SSL certificate file (default: None)"),
		click.option("--ssl-key-file", default=None, help="Path to SSL key file (default: None)"),

		# Mark --ssl-key-file as eager, so by the time the --ssl-cert-file validator is invoked,
		# we know if it's missing

	@click.option(
	"--no-interactive",
	is_flag=True,
	default=False,
	help="Disable interactive prompting for init parameters. (fail if any required values are missing)",
	cls=ClickMutex,
	required_param_lists=[
	["name", "location"],
	["name", "package_type", "base_image"],
	["name", "runtime", "dependency_manager", "app_template"],
	# check non_interactive_validation for additional validations
	],
	required_params_hint=REQUIRED_PARAMS_HINT,
	)

Add SSL support to local start-api and start-lambda #5902

Add SSL support to local start-api and start-lambda #5902

Conversation

mbklein commented Sep 7, 2023

Which issue(s) does this change fix?

Why is this change necessary?

How does it address the issue?

What side effects does this change have?

Mandatory Checklist

mndeveci commented Sep 8, 2023

mndeveci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbklein commented Nov 28, 2023

mndeveci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mndeveci left a comment

Choose a reason for hiding this comment

mbklein commented Dec 9, 2023

mbklein commented Dec 11, 2023

mndeveci commented Dec 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbklein commented Dec 12, 2023

lucashuy left a comment

Choose a reason for hiding this comment