UI and testing enhancements. #366
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 10 commits
September 14, 2022 07:35
With this, you can update the user component just by calling add_authenticator with the same credential_id but a different user. This will be a little more valuable in a later commit.
Because sometimes, you have multiple Webauthn tokens and the user wants to give each a name of some sort.
Building on the addition to registered authenticators, this adds support for adding, and using, aliases on authenticators. There are two points where we register an alias, the first is on device registration, and the second is on successful use. On the successful use case, we check to see if we already have an alias on the authenticator in question, and if we do not, we prompt for one. This makes it possible to track webauthn authenticators which were not registered via gimme_aws_creds.
Because there can be a noticible delay between the touch, and our next visible step, print a message to confirm that the touch was regisetred. This should help prevent things like accidentally triggering the Yubikey OTP output string, for users who don't notice that their device stopped blinking. (Like me, I definitely did that.)
If absent, or y, then we will use a system keyring if available. If it is n, then we will not use a system keyring even if it is available. This should prevent continual prompts for storing it in the system keyring for people who either do not wish to store it there, or for people whose corporate policies do not allow them to store it there.
A lot of search/replace made this go so much easier.
added 5 commits
September 20, 2022 20:01
This gets us off the old, no longer supported, beta release.
This is far easier to read, and even gives better status updates.
setup.py is depreciated, and recently broke, so let's just use pip install instead.
Among other things, this ensures that the tests all pass in a standardized enviornment, lacking anything specific to the machine the developer is running on.
|
@bwynsm Ping on a review? |
added 10 commits
October 5, 2022 05:01
This is entirely for a better user interface experience, getting asterisks back instead of nothing while typing a password may leak the length, but almost every single form that takes user input does this so a user gets feedback while they type. Since this includes the Okta web forms, this seems like a straight win.
Getting it this way has two important benefits: 1: It is already decoded. 2: When we support a single prompt for multiple webauthn keys, it will be the only possible way to know which key was used. 2 sounds like the more compelling reason, however the feature itself is of somewhat questionable value in that case.
This is because asserts get dropped if the code ever gets compiled to python bytecode. This is largely just a linter fix.
If we get a list of IDs, then we pass the whole list as the accept list, and if any of them are supported by a given device we can use it. This is required for being able to just ask the user if they want to use webauthn, instead of asking them to pick which webauthn device from a potentially unlabeled list that they want to use.
This is almost entirely to quiet the linter, however it also simplifies the code a little bit, and adds a comment to explain the code. Specifically, we should either check each and every layer of the object tree to see if the components exist, or we should rely on the AttributeError exception, not a combination of both. And we should only catch AttributeError, so we don't mask code problems in self._print_correct_answer.
Nothing to see here, just making the linter happier.
If we get DUO Web, only add DUO Push if we don't get DUO Push, and likewise only add DUO Passcode if we don't get DUO Passcode. And while we are at it, do both of the above even if we do get other MFA options, like Webauthn.
Now, preferred factors just replaces the factors list outright if we have any matches. And additionally, we now only prompt for factor selection if we have more than 1 possible MFA factor. (After all, if we are only given a single option, there's no point in ever prompting.)
Currently, if a user has 5 Webauthn keys configured in Okta (or, more likely, 2), the user gets 5 separate options in the factor list. This is... Not horribly user friendly, even if every single one does have a name configured for them. It turns out that Okta lets us ask for a nonce valid for every single one of them. If we take that path, we can take that nonce, and the whole list of possible challenge IDs, and pass it down to the webauthn code. At that point, we can let it pick whichever device is actually plugged in to this computer at this time. This means both that we can just give a single option in the factor list for Webauthn, and that setting the preferred MFA type to webauthn can Just Work even when a user has multiple webauthn tokens configured. This makes for a much better UI flow for users. The only big outstanding question I have is if the code around having names / labels for webauthn tokens makes any sense after this.
|
@Sector95 I don't suppose that you are still able to review PRs? :) |
added 2 commits
October 5, 2022 06:38
This makes the changes required to support the 1.0.0 and above releases of fido2. The biggest two changes are in having to implement fido2.client.UserInteraction instead of just passing in the PIN and an on_keepalive function callback, and in having to upgrade to a version of ctap-keyring-device which also supports fido2 1.0.0. For ctap-keyring-device, we have the complication that while 2.0.0 supports fido2 1.0.0, it seems that the 2.0.0 release hasn't actually been, well, released yet. As such, we're quite improperly importing directly from git right now. We should fix that.
The linter quite rightfully complains that parsing untrusted XML is not something done lightly.
added 22 commits
October 28, 2022 09:36
Little things...
It worked for me up until the first device was one that was registered.
This will be used for the case where no eligible webauthn devices are found, even if webauthn devices are found.
We now catch 'expected' errors from our webauthn code, cancel the operation with Okta, print an error message, and exit with a failure status. This isn't perfect, but it's definitely better.
After all, especially with session handling, it can be confusing on if we want you to touch your existing registered device, or a device to be registered.
Two things, first, we now always attempt to get the list of keyring devices, and if we are successful, we concat the two lists of devices. Second, we catch a known error in CtapKeyringDevice, and if we encounter it we print a message pointing at the PR which should fix the error.
self.locate_device() never raises an error, so we need to check for
_clients being empty ourselves, and if it is still empty after the
second try, we raise the error ourselves.
There is now a self._exception, which is a list of exceptions.
Functions run in threads are expected to append any errors to this list.
At the end, we check the list of errors.
We default to FIDODeviceTimeoutError('Operation timed out')
We then loop through the exception list, if we have instances of only
FIDODeviceTimeoutError, we keep the default exception, if we find an
instance of NoEligibleFIDODeviceFoundError, we switch to an error of
NoEligibleFIDODeviceFoundError('No eligible authentication devices
found.'), and if we have any other exception on the list, we pick that
one instead over all of the others, wrapping it in a FIDODeviceError.
We then throw whatever error we ended up with.
These errors are now caught by the calling okta code.
This should vastly improve the user experience around errors.
With current versions of python-fido2, this needed a fair bit of attention, and while I was at it, I started actually paying attention to a lot more of what Okta is sending us in regards to parameters for the credentials. There is also a workaround for a python-fido2 bug here, with a comment that has a link to the bug report.
Trivial conflict resolution around the README.md for the windows output format. The conflict resolution for requirements.txt was straight forward in that the version on this branch is needed for this branch. And for gimme_aws_creds/okta.py, the change to auto-select if there is only one factor just went away, as that was handled somewhat differently in the code on this branch. (preferred_factors gets collapsed into factors before we get here, and so we already check for only a single option)
|
line 93 of okta.py should be changed.
I think .. since the config default to '' at least thats my understanding looking through config.py Without this change I get a KeyError at that line of okta.py . the cookie isnt able to lookup the DT. |
|
also we might want to add a dep on a newer urllib3 .. I was getting an error unless I upgraded it. I kept throwing this error until I did that You need to be using urllib3 1.26+ to use allowed_methods. I recommend upgrading your Requests and urllib3 versions. |
Done, thank you. :) |
|
I hate to he that guy, but is there anything we can do to help move this along? Our company is introducing Yubikey for Okta and without this (or the other related) PR, that is really unergonomic to use on Mac. |
|
The new version of ctap-keyring-device hasn't been released yet. Once that's out we can get rid of the git reference in the requirements file and merge the changes. |
nijave
reviewed
Oct 19, 2023
| ctap-keyring-device@git+https://github.com/dany74q/ctap-keyring-device@de144921c6a0fb005422a6e3dc80966f01663e0a | ||
| pwinput>=1.0.2 | ||
| defusedxml | ||
| humps |
There was a problem hiding this comment.
I think you want pyhumps here https://humps.readthedocs.io/en/latest/user/install.html#install
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Related Issue
#362
#363
#364
#365
#322
#212
Motivation and Context
I would like to be able to recommend the switch to gimme-aws-creds in our internal environment, however without these changes I am currently unable to do so.
And I needed to switch to pytest so that I could test my changes successfully.
How Has This Been Tested?
This has been run locally, testing each change component, and then through the entire authentication process.
The test suite was converted to pytest, existing tests around registered authenticators were slightly modified to handle the argument and return value changes, and new tests were written for registered authenticators.
Screenshots (if appropriate):
Types of changes
Checklist: