New verify_ssl_certificate option

[kwessel]

• Allows skip of IdP SSL cert check when testing against a test IdP with a self-signed cert

[kwessel]

@ddriddle , I atmittedly got in over my head a bit with what I thought would be a simple feature suggestion. When I'm running awscli-login against my local dockerized IdP with a self signed cert, it obviously won't work. I thought it would be straightforward to add an option verify_ssl_cwertificate that defaults to True but could be changed to False for testing scenarios like mine. I think I made all of the right changes in the code, but I can't quite figure out the build. Make returns errors that confuse me. If you have some time to review this then talk through my changes with me and, in particular, how to compile it, I'd appreciate it. I also didn't bump the version number since, it appears, the version number being used is out of date, anyway. Hopefully, I didn't make my changes to an out-of-date version of the code.

[kwessel]

Well, Duh. Thanks, @edthedev ! I should have figured that out. Now, we'll see if my code changes actually work.

[kwessel]

@ddriddle and @edthedev , this is working now. I also suppressed the insecure request warning when you turn off SSL cert verification because, presumably, you know it's insecure if you're turning it off, and you're doing it for a reason.

I'm not sure if I like that you have to specify --verify-ssl-certificate=false on the command-line. It almost feels like it should be a switch: --skip-ssl-cert-verification which sets verify_ssl_certificate to false. But the --disable-refresh option works the same way my current code works, requiring a value of true or false. Do either of you have opinions on this?

We're also going to need another reviewer now that Ed has contributed. Who else should I add?

Thanks!

[ddriddle]

I'm not sure if I like that you have to specify --verify-ssl-certificate=false on the command-line. It almost feels like it should be a switch: --skip-ssl-cert-verification which sets verify_ssl_certificate to false. But the --disable-refresh option works the same way my current code works, requiring a value of true or false. Do either of you have opinions on this?

@kwessel I agree that --skip-ssl-cert-verification is more natural from the CLI but it is less natural in the config. I assumed most people folks would be putting these kind of options in the config file and all CLI flags are supported in the config file. This was done for consistency and brevity. So for those reasons I prefer --skip-ssl-cert-verification to work like force-refresh.

[edthedev]

My best guess on the failing test is some odd interaction with the 'or' here: https://github.com/techservicesillinois/awscli-login/blob/109d0dfa738f442048deda4eaa9dd084b3dd4579/src/awscli_login/config.py#L142-L143

But as far as I can tell it should work as written. ?

[kwessel]

@ddriddle , I agree that command-line switches would make more sense. I didn't do that because I wanted to be consistent with the refresh command-line option that you referenced. If we're going to change mine, we should change the other, as well. But, yes, specifying --skip-ssl-validation or something similar would be much more natural. I'll definitely take you up on a working session to make the minor changes and maybe to switch these two options to switches instead of options needing a true or false. If you have time next week, let's get on this. Thanks!

[edthedev]

I'm guessing the issue is here:

https://github.com/techservicesillinois/awscli-login/blob/109d0dfa738f442048deda4eaa9dd084b3dd4579/src/awscli_login/init.py#L103-L113

Note that verify-ssl-certificate has dashes while other versions have underscores.

[edthedev]

@ddriddle @kwessel I think I've found our unit test failure source. Per the comments, verify_ssl_certificate also needs a default value here.

As @ddriddle suggested, we should also capture this into an update to CONTRIBUTING.md guide for our future selves.

https://github.com/techservicesillinois/awscli-login/blob/53b18bb30bf06dc3e884c740253c51b4b2ae344a/src/tests/util.py#L101-L126

[edthedev]

@kwessel Here's the change I think will fix the unit tests, as a patch file:

From c6a72677f6bc2dd5dd2261922acbbb8ea1006216 Mon Sep 17 00:00:00 2001
From: Edward Delaporte <[email protected]>
Date: Wed, 18 May 2022 16:59:52 -0500
Subject: [PATCH] Fix for unit test failure.

---
 src/tests/util.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/tests/util.py b/src/tests/util.py
index 8d46eca..60f6c8a 100644
--- a/src/tests/util.py
+++ b/src/tests/util.py
@@ -113,6 +113,7 @@ def login_cli_args(
     duration=None,
     http_header_factor=None,
     http_header_passcode=None,
+    verify_ssl_certificate=True,
     # CLI only
     ask_password=False,
     force_refresh=False,
--
2.25.1

[edthedev]

@kwessel It just struck me that I could fork your fork to work on the unit tests, if you prefer, as well.

[kwessel]

@edthedev and @ddriddle , it's no secret that I could learn a thing or three from you guys.

I've (manually) made the change described in Ed's patch. Didn't even think about checking the tests to see if my new parameter needed to be reflected there. Thanks for catching that. Let's see if they pass now.

There are some other syntactical changes you all mentioned as well as the concept of moving the refresh and my new arg to be switches on the command line instead of needing a value. Can we meet next week to work through these?

Thanks!

[edthedev]

@kwessel Sorry this got stale. We're interested in rebasing and merging this code. Did you end up using your fork for this, or did you find a work-around? If a work-around, we're interested in adding it to the docs.