Skip to content
This repository has been archived by the owner on Sep 29, 2023. It is now read-only.

acquire_token_with_client_certificate with public cert failing #199

Closed
shgoldbe opened this issue Apr 4, 2019 · 5 comments · Fixed by #207
Closed

acquire_token_with_client_certificate with public cert failing #199

shgoldbe opened this issue Apr 4, 2019 · 5 comments · Fixed by #207

Comments

@shgoldbe
Copy link

shgoldbe commented Apr 4, 2019

providing a pem encoded public certificate for the parameter for authenticationcontext acquire_token_with_client_certificate fails to acquire token. Not providing a public_certificate succeeds. After following up with AAD team, it seems the request is malformed.

/adal/authentication_context.py line 237
Get Token request returned http error: 500 and server response: {"error":"server_error","error_description":"AADSTS50000: There was an error issuing a token.\r\nTrace ID: 03afcbc2-ba4b-4c7b-9b42-bdf114680100\r\nCorrelation ID: 392e482a-49ee-48c4-a45f-a7bd152ea3be\r\nTimestamp: 2019-04-04 18:12:51Z","error_codes":[50000],"timestamp":"2019-04-04 18:12:51Z","trace_id":"03afcbc2-ba4b-4c7b-9b42-bdf114680100","correlation_id":"392e482a-49ee-48c4-a45f-a7bd152ea3be"}

Is there a working example of this and/or can the library add verification of the contents of the public certificate aside from confirming that the headers are being constructed correctly?

@rayluo
Copy link
Collaborator

rayluo commented Apr 4, 2019

The working example for acquire_token_with_client_certificate() is here.

There is also a guildeline on how to work with the certificate.

Did you mean you use the public cerfiticate or the private certificate (i.e. PEM file) in acquire_token_with_client_certificate()?

@rayluo rayluo added the question label Apr 4, 2019
@shgoldbe
Copy link
Author

shgoldbe commented Apr 4, 2019

I have seen these examples, but they do not use public_certificate- I am trying to use SNI authentication.

I actually figured the issue out by having the AAD team dig through their server logs (and will fix this error case not to be 500). The issue is that in the initial PR to enable SN+I you expected a .pem encoded file for the public cert: a2524d7#diff-760dc384a167c4436779638a03c78900

and since removed the code to strip the pem headers: 9b58f3c#diff-6a2d410b3bd6c0538c7cc3911fc048b9

Can you update the function document comments to at least specify what format the public_certificate needs to be, since I assumed that the format should be the same as for the certificate (private key) and expect other consumers would make the same mistake.

@shgoldbe
Copy link
Author

shgoldbe commented Apr 4, 2019

For reference, this is part of my working code:

key = KeyvaultHelper().get_file_contents(cert_file_path, 'r')
public_cert = KeyvaultHelper().get_file_contents(public_cert_file_path, 'r')
thumbprint = KeyvaultHelper().get_public_key_thumbprint(public_cert_file_path)
resource = "https://someresource"

match = re.search(r'\-+BEGIN CERTIFICATE.+\-+(?P<public>[^-]+)\-+END CERTIFICATE.+\-+',public_cert, re.I)

public_certificate = match.group('public').strip()
token = auth_context.acquire_token_with_client_certificate(
    resource = resource,
    client_id = client_id,
    certificate = key,
    thumbprint = thumbprint,
    public_certificate=public_certificate)```

@rayluo
Copy link
Collaborator

rayluo commented Apr 4, 2019

Thanks Shelley for bringing this to our attention!

  • For now, app developers would have to follow the code snippet Shelley provided to get it work, because it was implemented that way.
  • We plan to add an improvement here in future. It will look for an optional "BEGIN CERTIFICATE" label, and extract its inner content if we would find one (new app devs would hit this route), or do nothing otherwise (this way our existing customers would not be break). This issue will remain open until we get that done.

@rayluo
Copy link
Collaborator

rayluo commented Jul 3, 2019

@shgoldbe Thanks again for bringing this to our attention. In next release of ADAL Python, we will accept the pem file with or without those tag lines, so that you won't have to remove your previous workaround. That being said, next time you want to rework your script, you can try SNI in our MSAL Python too!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants