-
-
Notifications
You must be signed in to change notification settings - Fork 545
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
#338 Add ID4me backend #339
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
please take a look into the comments.
social_core/backends/id4me.py
Outdated
self.validate_state() | ||
identity = self.strategy.session_get(self.name + '_identity') | ||
openid_configuration = self.get_identity_record(identity) | ||
if 'clp' not in openid_configuration: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RP shall never need to read out 'clp', as the user-info delegation to Agent shall rely on distributed claims
|
||
@handle_http_errors | ||
def user_data(self, access_token, *args, **kwargs): | ||
user_token = requests.get(self.userinfo_url(), headers={ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one requires rewrite to utilize distributed claims.
Essential is that initial access token can be only considered valid for authorities' userinfo, but the one to be used with Agent can be different and will be provided only by distributed claims object.
See also: https://gitlab.com/ID4me/general/issues/25
raise AuthUnreachableProvider(self) | ||
if not response: | ||
raise AuthUnreachableProvider(self) | ||
records = response[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth iterating all returned records
if not records: | ||
raise AuthUnreachableProvider(self) | ||
record = records[-1].strings[0].decode() | ||
return {item.split("=")[0]: item.split("=")[1] for item in record.split(";")} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there shall be a check that record has v=OID1 label set
association = self.get_association(iau) | ||
if not association: | ||
issuer_configuration = self.oidc_config_authority() | ||
response = requests.post(issuer_configuration['registration_endpoint'], json={ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there shall be some configuration added for policy/logo/tos URLs
def get_key_and_secret(self): | ||
iau = self.strategy.session_get(self.name + '_authority') | ||
association = self.get_association(iau) | ||
if not association: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there shall be a check, that registration did not expire (client_secret_expires_at)
def get_identity_record(self, identity): | ||
try: | ||
response = dns.resolver.query('_openid.' + identity, 'TXT', lifetime=5).response.answer | ||
except NXDOMAIN or Timeout: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There shall be a lookup on parent domain in case of failure.
See https://gitlab.com/ID4me/documentation/blob/master/id4ME Technical Specification.adoc 2.1.2
In case _openid subdomain cannot be resolved, the record for the parent domain SHALL be resolved, by removing the leftmost label from the original ID4me Identifier.
social_core/backends/id4me.py
Outdated
header = jwt.get_unverified_header(id_token) | ||
if header['kid'] == key['kid']: | ||
if 'alg' not in key: | ||
key['alg'] = 'RS256' if key['kty'] == 'RSA' else 'ES256' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typically there shall be a correlation between id_token_signed_response_alg
set in the client registration and the signature key. If omitted they default to 'RSA256', so usage of 'ES256' is not correct in this case.
social_core/backends/id4me.py
Outdated
header = jwt.get_unverified_header(id_token) | ||
if header['kid'] == key['kid']: | ||
if 'alg' not in key: | ||
key['alg'] = 'RS256' if key['kty'] == 'RSA' else 'ES256' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typically there shall be a correlation between userinfo_signed_response_alg
set in the client registration and the signature key. If omitted they default to 'RSA256', so usage of 'ES256' is not correct in this case.
social_core/backends/id4me.py
Outdated
raise AuthTokenError(self, 'Incorrect id_token: nbf') | ||
|
||
# Verify the token was issued in the last 10 minutes | ||
iat_leeway = self.setting('ID_TOKEN_MAX_AGE', self.ID_TOKEN_MAX_AGE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this arbitrary check for 10 minutes? There is exp
claim which shall be respected.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
No description provided.