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

#338 Add ID4me backend #339

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

andreeadima
Copy link

No description provided.

Copy link

@pawel-kow pawel-kow left a 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.

self.validate_state()
identity = self.strategy.session_get(self.name + '_identity')
openid_configuration = self.get_identity_record(identity)
if 'clp' not in openid_configuration:

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={

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]

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(";")}

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={

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:

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:

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.

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'

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.

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'

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.

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)

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.

@stale
Copy link

stale bot commented Mar 22, 2020

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.

@stale stale bot added the stale Stale issues (closing soon) label Mar 22, 2020
@stale stale bot closed this Mar 29, 2020
@omab omab reopened this Jan 9, 2021
@stale stale bot removed the stale Stale issues (closing soon) label Jan 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants