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

Feature: Accept Asymmetric Algorithm #73

Merged
merged 17 commits into from Mar 16, 2022

Conversation

filwaline
Copy link
Contributor

I implemented SymmetricSecret and AsymmetricSecret, and drop replace it.

New tests all passed, and fully covered.

I am poor in English, so docs remain old.

@filwaline
Copy link
Contributor Author

filwaline commented Feb 26, 2022

For #70

LoginManager now accept single secret still, nothing is broken. But It also accept dict, for asymmetric purpose.

# random string or private-key(pem format)
random_string = ...  
private_key_pem_bytes = ...

# algorithm must match secret type
# random string & HS256
LoginManager(secret=random_string, algorithm="HS256", ...)
# private-key & RS256
LoginManager(secret=private_key_pem_bytes, algorithm="RS256", ...)
# dict secret
LoginManager(secret={"private_key":private_key_pem_bytes, "password": ...}, algorithm="RS256", ...)

@filwaline
Copy link
Contributor Author

I am using black as code formatter, lots of changes just formatting.

@filwaline filwaline changed the base branch from master to development February 26, 2022 13:48
Copy link
Owner

@MushroomMaula MushroomMaula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good. Thanks a lot for your help.
Maybe you can answer the two questions regarding my comments.

Comment on lines +14 to +16
class RawPrivateSecret(BaseModel):
private_key: SecretBytes
password: Optional[bytes] = None
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give an example of how this class should be used?
I assume this is one way the input can be given to the secret parameter for LoginManager when using asymmetric keys? If so I think it is a good idea to add a test case that uses this class in addition to the dictionary approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@filwaline filwaline Mar 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parsing are handled by pydantic model AsymmetricSecret, so RawPrivateSecret will be take care and should not use directly.

RawPrivateSecret is part of AsymmetricSecretIn, and the latter one is used to pre-validate AsymmetricSecret's secret field.
https://github.com/filwaline/fastapi_login/blob/0f0dc21d9b07f35a6b5c909c4e44d89602ecfe5d/fastapi_login/secrets.py#L45-L48

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But RawPrivateSecret is never invoked directly? In AsymmetricSecret data is of type AsymmetricPairKey, this is also mismatch of the type when passing to AsymmetricSecretIn as the data parameter here expects either SecretBytes or RawPrivateSecret, therefore I don't see how or why pydantic should cast the given value to either one of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the flag pre=True indicated that it is a pre-validator.
Therefore AsymmetricSecret haven't paring raw data into AsymmetricPairKey yet, not until the end of pre-validator secret_must_be_asymmetric_private_key, AsymmetricSecretIn is actually dealing with raw data.


A step-by-step debug process will show you how data flows, it is better than plain text.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

secret_must_be_asymmetric_private_key is handling three different jobs:

  1. validate the syntax of raw data
  2. validate if given data is a valid asymmetric key
  3. derive it's public key and cast into AsymmetricPairKey

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I got it now.
If one passes {"private_key": "...", "password": "..."} to AsymmetricSecret, this is then passed onto AsymmetricSecretIn, which in turn is recognized by pydantic as having the same fields as RawPrivateSecret and is thus validated as such. If all of this passes the public key is derived from the private key and then cast together as AsymmetricPairKey.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I got it now. If one passes {"private_key": "...", "password": "..."} to AsymmetricSecret, this is then passed onto AsymmetricSecretIn, which in turn is recognized by pydantic as having the same fields as RawPrivateSecret and is thus validated as such. If all of this passes the public key is derived from the private key and then cast together as AsymmetricPairKey.

Yes, that what I meant!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current code only accept PEM format key, do you think we should accept other formats?
For example, DER format.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think PEM is sufficient for now, if the other formats are needed it shouldn't be too much trouble adding them later on.

tests/test_secret.py Show resolved Hide resolved
@MushroomMaula
Copy link
Owner

Yeah I saw that, but I think if the support is there it doesn't hurt to make it available directly.

@MushroomMaula
Copy link
Owner

On the other hand it is probably a good idea to not include it if it isn't needed 🤔 . If so, I think it would be helpful to show a message that cryptography needs to installed when a key pair is passed as the secret.

@filwaline
Copy link
Contributor Author

filwaline commented Mar 10, 2022

On the other hand it is probably a good idea to not include it if it isn't needed 🤔 . If so, I think it would be helpful to show a message that cryptography needs to installed when a key pair is passed as the secret.

Current message is a pydantic validation error that algorithm"RS256" is invalid, if user selected "RS256" but not installed cryptography.

Maybe you can add some documents about it, just like FastAPI Form Data

@MushroomMaula
Copy link
Owner

I will add some documentation in the following days explaining the workflow with asymmetric keys and will then merge and publish a new package version.
Thanks a lot for your help so far.

@MushroomMaula MushroomMaula merged commit 3312b80 into MushroomMaula:development Mar 16, 2022
@filwaline filwaline deleted the feature-asymmetric branch March 16, 2022 10:53
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

3 participants