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

Implements certificate credential authentication using JWT assertions #247

Merged
merged 5 commits into from
May 25, 2024

Conversation

jannispl
Copy link
Contributor

@jannispl jannispl commented May 6, 2024

Since Microsoft is no longer issuing client secrets with indefinite validity for O365 application registrations, I have decided to implement certificate credential authentication using JWT assertions. At this time, there seem to be no restrictions on the validity of self-signed certificates (I managed to upload and use a 10yr certificate to Entra just fine).

Microsoft even recommends certificate authentication:

For best security, we recommend using certificate credentials.

The workflow and technicals for certificate authentication are described here.

This PR adds two account configuration options jwt_certificate_path and jwt_key_path. These options are to be used in place of client_secret (i.e., that value should be removed). jwt_certificate_path is expected to be a path to a x509 pem-encoded certificate; jwt_key_path is expected to be a path to an unencrypted pem-encoded private key.

To generate the certificate and private key, the following command can be used:

openssl req -x509 -newkey rsa:4096 -keyout key.pem -out certificate.pem -sha256 -days 3650 -nodes -subj "/CN=email-oauth2-proxy"

The certificate.pem that is generated can then be uploaded to the application registration in the Entra admin console.

@simonrob
Copy link
Owner

simonrob commented May 9, 2024

Thanks for the contribution!

In order to test before merging this I'll need access to an account that can be authenticated via JWT, which I don't have at the moment. Is this something you could provide for this purpose?

@jannispl
Copy link
Contributor Author

I could provide you with a temporary test account. How can I contact you privately?

@simonrob
Copy link
Owner

That would be perfect. You can reach me via my GitHub username at Gmail.

@jannispl
Copy link
Contributor Author

I have sent you an email.

@simonrob
Copy link
Owner

Thanks for providing the test account – I've just taken a look. I made a few minor edits:

  • Remove the edit to pyproject.toml as this is not needed (but clarify the message here for future reference)
  • Lower the minimum pyjwt version to 2.4 so that Python 3.6 is still supported
  • Variable renaming (including in the configuration file) for consistency with the rest of the proxy
  • Lint reformatting to match the project style
  • Remove some of the JWT parameters that RFC 7519 specifies as optional and O365 does not require
  • Add basic documentation

Just so there is a record, here's a sample configuration file entry using this method:

[[email protected]]
permission_url = https://login.microsoftonline.com/*** your tenant id here ***/oauth2/v2.0/authorize
token_url = https://login.microsoftonline.com/*** your tenant id here ***/oauth2/v2.0/token
oauth2_scope = https://outlook.office365.com/IMAP.AccessAsUser.All https://outlook.office365.com/SMTP.Send offline_access
redirect_uri = http://localhost
client_id = *** your client id here ***
jwt_certificate_path = /path/to/certificate.pem
jwt_key_path = /path/to/key.pem

Unless there are any other features to be added, I think this is probably ready to be merged. Let me know what you think?

@jannispl
Copy link
Contributor Author

Thanks for taking your time to polish this - it's looking really good now.

Re:

  • Remove some of the JWT parameters that RFC 7519 specifies as optional and O365 does not require

They might not be required by the spec, but at least for jti Microsoft's documentation is worded a little more strict:

The identifier value must be assigned in a manner that ensures that there's a negligible probability that the same value will be accidentally assigned to a different data object [...]

I also found this page which states:

The claims expected by Microsoft Entra ID in the signed assertion are: [...]

and

If you want to provide your own claims, including the mandatory claims expected by Microsoft Entra ID, pass in false for the mergeWithDefaultClaims parameter.

@simonrob
Copy link
Owner

Interesting – thanks for following up. The original page isn't completely clear – it essentially just lists the properties defined in the RFC. Because of this, I determined through experimentation which ones O365 does and does not actually require for OAuth 2.0 login. I hadn't seen the other link you've found, but in its Crafting the assertion section it gives this example:

// no need to add exp, nbf as JsonWebTokenHandler will add them by default.
var claims = new Dictionary<string, object>()
{
    { "aud", tokenEndpoint },
    { "iss", clientId },
    { "jti", Guid.NewGuid().ToString() },
    { "sub", clientId }
};

As explained in the comment, the exp property is added automatically by JsonWebTokenHandler in this example. The proxy currently sets a 5-minute expiry time here. Interestingly, Microsoft's default expiry time is 60 minutes, though their documentation suggests using a value of 5-10 minutes at most. It's safe to stick to the proxy's current approach for this property, I think.

From testing, I found that the nbf property is not actually required by O365, though I agree that we should probably add it back since it seems like it may be expected, and the default that O365 sets this value to (the Unix epoch) does not seem sensible. The iat property is not mentioned on this page, and not currently added in JsonWebTokenHandler either (though that hasn't always been the case).

So: long story short, I think nbf and jti should be added, but we can leave iat out. What do you think?

@jannispl
Copy link
Contributor Author

So: long story short, I think nbf and jti should be added, but we can leave iat out. What do you think?

Sounds reasonable to me.

@simonrob simonrob merged commit c96efdb into simonrob:main May 25, 2024
@jannispl jannispl deleted the jwt-assertions branch May 25, 2024 18:57
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.

None yet

2 participants