Feature/b2c support

[robteeuwen]

Added support for tokens without x5c fields, and added optional openid_config_url override, this would close #46

Not all tests are working yet, not sure if that is due to the code itself or due to the test config

[JonasKs]

Thank you so much! I’ll look into tests as soon as I can, but it might take a few days. 😊

[JonasKs]

You could look into creating the signing key from jwk instead of manually like I’ve done? Seems like it loads a bit more than just the public key.

[JonasKs]

Hi @robteeuwen !

I did some research over lunch and think I figured it out. In short, n == modulus and e == exponent, and with these two one can generate the public key. These are not generated properly through your code.

These matters so much now, because x5c is completely ignored by python-jose, as I've described in detail over in this issue.

Since Azure keys always consist of both n+e and x5c, we can get away with removing the entire x5c from our mocked keys endpoint. However, we need to generate the n and e properly. Luckily, this is pretty easy, and most of the code needed is already provided in utils.py here.

Wrap that in construct and success:

jwk.construct(
    signing_key_b.private_bytes(
        crypto_serialization.Encoding.PEM,
        crypto_serialization.PrivateFormat.PKCS8,
        crypto_serialization.NoEncryption(),
    ),
    'RS256',
).to_dict()

Let me know if anything is unclear. 😄

[JonasKs]

I wanted to learn how to actually convert n and e properly, and this is the solution:

base64.b64encode(signing_key_b.public_key().public_numbers().e.to_bytes(((signing_key_b.public_key().public_numbers().e.bit_length() + 7) // 8), 'big'))
base64.b64encode(signing_key_b.public_key().public_numbers().n.to_bytes(((signing_key_b.public_key().public_numbers().n.bit_length() + 7) // 8), 'big'))

Where n.to_bytes(((signing_key_b.public_key().public_numbers().n.bit_length() + 7) // 8), 'big') is stolen from here.

The RFC says

The "n" (modulus) parameter contains the modulus value for the RSA
public key. It is represented as a Base64urlUInt-encoded value.

which under Terminology can be read about:

Base64urlUInt
The representation of a positive or zero integer value as the
base64url encoding of the value's unsigned big-endian
representation as an octet sequence. The octet sequence MUST
utilize the minimum number of octets needed to represent the
value. Zero is represented as BASE64URL(single zero-valued
octet), which is "AA".

and now my head is boiling. Looking at the python-jose code, I can't find this solution, so I am sure it is a better way. 😄

[robteeuwen]

hey Jonas, thanks so much for looking into this. I'm not sure I'm following completely. I tried to make some changes like the ones you suggest, but my tests are still failing. So first things first, do you think I'm generating the cert_obj incorrectly in the application code (here) or do you think they keys are only generated incorrectly in the tests?

[JonasKs]

@robteeuwen , I've commited to your branch which should fix it. Let me know if you want me to describe the changes in detail.

I also ran linting. Please run pre-commit install locally, it will ensure checks are run and passed before you're able to commit new changes :)

EDIT: This was a great article about these concepts.

[codecov]

Codecov Report

Merging #48 (895dfb6) into main (6386890) will not change coverage.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##             main      #48   +/-   ##
=======================================
  Coverage   100.0%   100.0%           
=======================================
  Files           5        5           
  Lines         173      174    +1     
=======================================
+ Hits          173      174    +1     

Impacted Files	Coverage Δ
fastapi_azure_auth/__init__.py	100.0% <100.0%> (ø)
fastapi_azure_auth/auth.py	100.0% <100.0%> (ø)
fastapi_azure_auth/openid_config.py	100.0% <100.0%> (ø)

[JonasKs]

Why is it required to pass in config_url? How does this endpoint look for B2C? Could we simplify it? At the moment we have SingleTenant and MultiTenant, could we add one B2C class which configures this nicely instead?

[robteeuwen]

That's a good question. In my organization's case the domain is different for B2C (it's a custom domain). This may be non standard, but in general, I think it's nice to have the option to override the config_url, since you can also override the other urls. But I set it to optional, so it's not required to pass it.

In case this is non-standard B2C behavior, I guess it doesn't technically belong to the same feature as adding B2C support, so maybe it belongs in a different PR.

Another thing that's different is that B2C openid config url includes a policy: https://docs.microsoft.com/en-us/azure/active-directory-b2c/openid-connect

[JonasKs]

Aha, so you can customize that URL in B2C, I didn't know. In the normal single- and multi-tenant situations these can be generated by the other settings.
Because of this, I think I'd like that we alter the base to accept config_url (as you've done), and then create a new B2CAuthorizationCodeBearer-class which only expose functionality you need in B2C scenarios (like I've done with the single- and multi-tenant classes). I like this design, because anyone who wish to alter everything can use the AzureAuthorizationCodeBearerBase directly, but for normal users they can use a custom class for their need that only has an API for settings they should care about.

As for the policy: anything in particular you think would require code changes to support?
I strive to keep the scope of this package as small as possible, which is validation of tokens, so that security is always the only priority. 😊

Sorry for the wall of text. 😅

EDIT: I see what you mean about policies now. Should be mostly about extending the config_url indeed. Would love to generate that in the B2CAuthorizationCodeBearer class, which passes on a newly created config_url to the AzureAuthorizationCodeBearerBase.

[JonasKs]

Hi @robteeuwen, do you have any questions or want me to help out with something? 😊

[JonasKs]

Hi @robteeuwen , I’ll finish up this PR with the current changes this week if I don’t hear back. The specific B2C class and docs can be another PR, this PR is a nice start to all of that.

[robteeuwen]

Hi @robteeuwen , I’ll finish up this PR with the current changes this week if I don’t hear back. The specific B2C class and docs can be another PR, this PR is a nice start to all of that.

Hi Jonas, I'm sorry for bailing on this, I've been traveling and some other things came up that prevented me from finishing this up. It looks like only the code coverage check is failing, so we're missing a test somewhere? Or did you want me to make other changes to the code? If you could help finishing this up that would be great, otherwise I can get back to it myself in the next couple of weeks.

[JonasKs]

No worries! I’ll see what I can do 😊

Yeah, a test or two is missing. Would also love to include documentation(like I have for single- and multi-tenant applications) for B2C, but don’t really need that to merge this.

[JonasKs]

I've removed the setting from SingleTenantAzureAuthorizationCodeBearer and MultiTenantAzureAuthorizationCodeBearer, so that you have to use the AzureAuthorizationCodeBearerBase directly. I have not found any case where this is required for normal tenants, only for B2C, so don't see a reason to expose the setting.

I've added a test and cleaned up a little. Will merge and release ASAP.

[JonasKs]

I've merged and creates a release - thank you so much!

I would love documentation(like we got for single- and multi-tenant apps) and a specific B2C-class in the future, if you're interested in helping out with that😊

[lsmith77]

@robteeuwen thank you for this! do you have some documentation or at least some example code/config you can share to get this setup?

[lsmith77]

for now I am seeing if using https://github.com/425show/fastapi_microsoft_identity#2-azure-ad-b2c-authentication gets me started in the right direction