Skip to content

Human message regarding minimum client_secret lenght #72

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

Merged
merged 1 commit into from
Oct 17, 2020

Conversation

peppelinux
Copy link
Member

@peppelinux peppelinux commented Oct 16, 2020

A simple commit that wants to focus on an error message reported to users.

Standing on CSCfi shib oidc op guide the example client_secret is topsecret. That's something that users tends to use as it is for test setups.

cryptojwt instead, have a default minimum length secret, as a policy, it seems quite hardcoded here:

I purpose a human readable message in this PR, to drive the users in a correct configuration (just to understand how thing should be done to get it to work). At the same time I'd think also to a global configuration parameters that could set that minimum value, an approch like:

min_secret_len = getattr(general_config, 'min_secret_len', 16)

This is a WiP, probably I'd add others stuffs here.
that's also a footprint on the test I made:

image

@peppelinux peppelinux force-pushed the human_message_secret_len branch from 38c23a0 to a942aed Compare October 16, 2020 11:33
@peppelinux peppelinux force-pushed the human_message_secret_len branch from a942aed to f8cfa89 Compare October 16, 2020 13:15
@codecov-io
Copy link

codecov-io commented Oct 16, 2020

Codecov Report

Merging #72 into develop will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop      #72   +/-   ##
========================================
  Coverage    78.39%   78.39%           
========================================
  Files           41       41           
  Lines         4193     4193           
  Branches       808      808           
========================================
  Hits          3287     3287           
  Misses         648      648           
  Partials       258      258           
Impacted Files Coverage Δ
src/cryptojwt/jwk/hmac.py 81.39% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 030b1c6...f8cfa89. Read the comment docs.

@rohe rohe merged commit 61d1e9d into develop Oct 17, 2020
@rohe
Copy link
Contributor

rohe commented Oct 17, 2020

By the way, we/I should tell Henri about this !

@peppelinux
Copy link
Member Author

I'm working on a concise Paper with some general warnings about Shib CSCfi OIDC configuration. That's something to submit for revision of collegues that develops shibboleth,I will notify you on this before that happens

@rohe
Copy link
Contributor

rohe commented Oct 18, 2020

I'm sure Henri will very much approve of this.

@peppelinux
Copy link
Member Author

I'm sure Henri will very much approve of this.

Good news.
At the sane time I didn't get idpy rp to work with cscfi op (access token fails signature validation RP side). IHope to have a deeper look in It, let me know of you've interest in this compatibility test, After all It looks quite strange

@rohe
Copy link
Contributor

rohe commented Oct 19, 2020

Access token signature validation ? You must mean identity token signature validation.

@peppelinux
Copy link
Member Author

Access token signature validation ? You must mean identity token signature validation.

Tracelog on slack #oidc

@rohe
Copy link
Contributor

rohe commented Oct 19, 2020

Well, the error is slightly of target. If you look a bit further up on the trace you see
idt = IdToken().from_jwt(str(msg[claim]), **args)
That's really what's happening.
Don't know where AccessToken comes from.

@peppelinux
Copy link
Member Author

Right, I have to to do something for that, really don't know how to handle this exception

@peppelinux
Copy link
Member Author

The CSCfitutorial they made shows us how to create some JWKs.
These are not related to the preexisting RSA key/cert (in PEM format) used by IdP to sign payloads, but something different, not involved in the real jws operations.I was able to verify the tokens by creating a JWK starting from idp's cert/key in PEM format.
So I create some new jwks with this, and it works

import json
from cryptojwt.jwk.x509 import import_public_key_from_cert_file
from cryptojwt.jwk.rsa import import_private_rsa_key_from_file
from cryptojwt.jwk.rsa import RSAKeyIDP_HOME = "/opt/shibboleth-idp"private = import_private_rsa_key_from_file(f'{IDP_HOME}/credentials/idp-signing.key')
rsa_key = RSAKey(priv_key=private)
print(json.dumps(rsa_key.serialize(), indent=2))

@jschlyter jschlyter deleted the human_message_secret_len branch March 17, 2021 10:25
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.

3 participants