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.

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

Update to work with new fido2 version #33

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

keis
Copy link

@keis keis commented Jan 27, 2022

Not working yet and I don't know enough about okta to figure it out easily

@coldcoff
Copy link
Contributor

Seems to be the interface changes to get_assertion in upstream python-fido2 version 0.8.0 and 0.9.0
https://github.com/Yubico/python-fido2/blob/58471d4af1c09cc9fe316771d6203b4844cbc668/NEWS

Do you want to continue to work on a PR? I can carry on as well, if you like, but I am also happy to just peer-review it and leave that PR to you.

@keis
Copy link
Author

keis commented Jan 27, 2022

With these changes it should be up to date with that api change I think but something is still not working.

I'll probably poke at it some more between meetings and what not, but feel free to have a go at it.

@coldcoff
Copy link
Contributor

Thanks, I'll take my go in the evening. Not good to play with the VPN in the middle of my working day :-)

@arthepsy
Copy link
Owner

I currently don't have my hands on Fido2 to test and help, but I could review the PR, when it's done.
If I get my hands on one, does it need some additional config on OKTA side?

@coldcoff
Copy link
Contributor

coldcoff commented Jan 28, 2022

I played without success several hours now :-(

I think your change is good and does the right thing to upgrade python-fido2 from 0.7.0 to 0.9.3

But: it doesn't work for me finally, either.

2 minor remarks:

  • From reading the examples in https://github.com/Yubico/python-fido2 I think it is "better" to use camelCase here:

    result = client.get_assertion({
    	'challenge': challenge.encode('utf-8'),
    	'rpId': purl[1],
    	'allowCredentials': allow_list
    })
    

    ... but that should not matter as there seems to be code to accept both spellings.

  • Additionally, also from reading the examples, I think the result handling looks better as:

    response = result.get_response(0) # only one cred in allowList, so only one response.
    data = {
    	'stateToken': state_token,
    	'clientData': to_n((base64.b64encode(response.client_data)).decode('ascii')),
    	'signatureData': to_n((base64.b64encode(response.signature)).decode('ascii')),
    	'authenticatorData': to_n((base64.b64encode(response.authenticator_data)).decode('ascii'))
    }
    

    (this saves accessing the "private" assertion._client_data in your code and uses the new result object)

... but that's all only cosmetics it seems. Main issue is that it does not work anymore. Those fido2 steps all seem to succeed. We get POST data for Okta without issues and they look at least sensible. But when I post the result to Okta API they respond with a 503 and an error with something like:

{"errorCode":"E0000068","errorSummary":"Invalid Passcode/Answer","errorLink":"E0000068","errorId":"oaesNxKuS33RFeyOwaJXNyJ1w","errorCauses":[{"errorSummary":"Invalid nonce."}]}

I assume it's the same picture on your side when you say "not working yet"?

Probably we need to cut the fido code out to some dedicated test program and try to get some stable input (might be easy) and output (might be hard due to the use case...) to see what's the difference in detail when changing 0.7.0 <-> 0.9.3.

I'm reverting to 0.7.0 for the time being, need to show up at work :-)

Let's see I'll find some time to continue playing over the weekend. At least the fix it not urgent, we have some time. But we should make it work again, with recent python-fido2.

@keis
Copy link
Author

keis commented Jan 28, 2022

yes, that's the same thing I'm seeing there's some activity and the fido2 calls as far as I can tell are working but then I get that "Invalid Passcode/Answer" from OKTA.

@coldcoff
Copy link
Contributor

Got it!!!!!
(I peeked a bit at https://github.com/Nike-Inc/gimme-aws-creds)

Here is my diff against master:

diff --git a/gp-okta.py b/gp-okta.py
index 24d4662..3907884 100755
--- a/gp-okta.py
+++ b/gp-okta.py
@@ -8,7 +8,8 @@
    Copyright (C) 2019 Aaron Lindsay ([email protected])
    Copyright (C) 2019 Taylor Dean ([email protected])
    Copyright (C) 2020 Max Lanin ([email protected])
-   Copyright (C) 2019-2020 Tino Lange ([email protected])
+   Copyright (C) 2019-2022 Tino Lange ([email protected])
+   Copyright (C) 2022 David Keijser ([email protected])
 
    Permission is hereby granted, free of charge, to any person obtaining a copy
    of this software and associated documentation files (the "Software"), to deal
@@ -56,6 +57,7 @@ try:
 	from fido2.utils import websafe_decode
 	from fido2.hid import CtapHidDevice
 	from fido2.client import Fido2Client
+	from fido2.webauthn import PublicKeyCredentialRequestOptions, PublicKeyCredentialDescriptor, PublicKeyCredentialType
 	have_fido = True
 except ImportError:
 	pass
@@ -120,13 +122,13 @@ def warn(s):
 		print(u'[WARN] {0}'.format(s))
 
 def dbg(d, h, *xs):
-	# type: (Any, str, Union[str, List[str], Dict[str, Any]]) -> None
+	# type: (Any, str, Union[str, List[str], Tuple[str], Dict[str, Any]]) -> None
 	if quiet:
 		return
 	if not d:
 		return
 	for x in xs:
-		if not isinstance(x, dict) and not isinstance(x, list):
+		if not isinstance(x, dict, list, tuple):
 			for line in x.split('\n'):
 				print(u'[DEBUG] {0}: {1}'.format(h, line))
 		else:
@@ -649,27 +651,30 @@ def okta_mfa_webauthn(conf, factor, state_token):
 	profile = rfactor['profile']
 	purl = parse_url(conf.okta_url)
 	origin = '{0}://{1}'.format(purl[0], purl[1])
-	challenge = rfactor['_embedded']['challenge']['challenge']
-	credentialId = websafe_decode(profile['credentialId'])
-	allow_list = [{'type': 'public-key', 'id': credentialId}]
+	request_options = PublicKeyCredentialRequestOptions(
+		challenge = websafe_decode(rfactor['_embedded']['challenge']['challenge']),
+		rp_id = purl[1],
+		allow_credentials = [PublicKeyCredentialDescriptor(PublicKeyCredentialType.PUBLIC_KEY,
+			websafe_decode(profile['credentialId']))]
+	)
 	for dev in devices:
 		client = Fido2Client(dev, origin)
 		print('!!! Touch the flashing U2F device to authenticate... !!!')
 		try:
-			result = client.get_assertion(purl[1], challenge, allow_list)
-			dbg(conf.debug, 'assertion.result', result)
+			result = client.get_assertion(request_options)
+			dbg(conf.debug, 'assertion.result', vars(result))
 			break
 		except Exception:
 			traceback.print_exc(file=sys.stderr)
 			result = None
 	if not result:
 		return None
-	assertion, client_data = result[0][0], result[1] # only one cred in allowList, so only one response.
+	response = result.get_response(0) # only one cred in allow_credentials, so only one response.
 	data = {
 		'stateToken': state_token,
-		'clientData': to_n((base64.b64encode(client_data)).decode('ascii')),
-		'signatureData': to_n((base64.b64encode(assertion.signature)).decode('ascii')),
-		'authenticatorData': to_n((base64.b64encode(assertion.auth_data)).decode('ascii'))
+		'clientData': to_n((base64.b64encode(response.client_data)).decode('ascii')),
+		'signatureData': to_n((base64.b64encode(response.signature)).decode('ascii')),
+		'authenticatorData': to_n((base64.b64encode(response.authenticator_data)).decode('ascii'))
 	}
 	log('mfa {0} signature request [okta_url]'.format(provider))
 	_, _h, j = send_json_req(conf, 'okta', 'uf2 mfa signature', j['_links']['next']['href'], data, expected_url=conf.okta_url)

@keis
Copy link
Author

keis commented Jan 28, 2022

w00t that's awesome!

Want me to update this PR with that, or perhaps it's easier for you to open a new one instead?

@coldcoff
Copy link
Contributor

Let's not waste PRs and forks ...

Please integrate in your branch, test & adapt the PR yourself (maybe you have something more to add/change?) [also I did not test if it still works on py2, btw] -- and get @arthepsy merge that ;-)

Besides: LGTM from my side. Thanks for bringing that up!

@keis
Copy link
Author

keis commented Jan 28, 2022

Tested with both python2.7 and 3.9

@keis keis changed the title WIP Fido2 update Update to work with new fido2 version Jan 28, 2022
gp-okta.py Show resolved Hide resolved
@coldcoff
Copy link
Contributor

coldcoff commented Feb 1, 2022

@arthepsy -- IMHO this can be merged.
I'm using that new code every day now, works!

@keis
Copy link
Author

keis commented Feb 10, 2022

bump :)

@coldcoff
Copy link
Contributor

@arthepsy -- this can be merged. works like a charm. any remaining objections?

@coldcoff
Copy link
Contributor

Ping?

@eedgar
Copy link

eedgar commented Apr 5, 2022

@keis perhaps you could consider adding some of the changes from #35 in to this pr as well.. pin support, automatically selecting the right key if you have multiple key in the computer etc..

keis and others added 3 commits September 26, 2022 11:39
In version 0.9.0 of fido2 the API is changed to accept/return new
request/response objects.

Co-authored-by: Tino Lange <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants