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

Add MFA capability and permission #79

Merged
merged 7 commits into from Mar 18, 2024
Merged

Conversation

mickenordin
Copy link
Contributor

This PR is a proposal to add a capability /mfa-enforcing as well as a permission mfa-enforced to the specification. The version of the specification is bumped to 1.2.0.

If an OCM provider has the capability /mfa-enforcing it will respond with a boolean on the endpoint /mfa-enforcing to indicate whether or not it will try to comply with a MFA requirement set as a permission on a share. If the sharer OCM provider trusts the sharee OCM provider the sharer MAY set the permission mfa-enforced on a share.

A complient OCM provider that signals mfa-enforcing true MUST not allow access to a resource to a user that has not provided a second factor to establish the identity of the user with greater confidence.

Since there is no way to guarantee that the sharee OCM provider will actually enforce the MFA requirement, it is up to the sharer OCM provider to establish a trust with the OCM sharee provider such that it is reasonable to assume that the sharee OCM provider will honor the MFA requirement. This establishment of trust will inevitably be implementation dependent, and can be done for example using a pre approved allow list of trusted OCM providers. The procedure of establishing trust is out of scope for this specification.

This PR is a proposal to add a capability `/mfa-enforcing` as well as
a permission `mfa-enforced` to the specification. The version of the
specification is bumped to 1.2.0.

If an OCM provider has the capability `/mfa-enforcing` it will respond
with a boolean on the endpoint /mfa-enforcing to indicate whether or
not it will try to comply with a MFA requirement set as a permission
on a share. If the sharer OCM provider trusts the sharee OCM provider
the sharer MAY set the permission `mfa-enforced` on a share.

A complient OCM provider that signals mfa-enforcing `true` MUST not
allow access to a resource to a user that has not provided a second
factor to establish the identity of the user with greater confidence.

Since there is no way to guarantee that the sharee OCM provider will
actually enforce the MFA requirement, it is up to the sharer OCM
provider to establish a trust with the OCM sharee provider such that
it is reasonable to assume that the sharee OCM provider will honor the
MFA requirement. This establishment of trust will inevitably be
implementation dependent, and can be done for example using a pre
approved allow list of trusted OCM providers. The procedure of
establishing trust is out of scope for this specification.
@mickenordin mickenordin marked this pull request as draft January 12, 2024 11:27
Copy link
Member

@glpatcern glpatcern left a comment

Choose a reason for hiding this comment

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

I like the approach, including the additional capability a provider is expected to implement.

I think part of the description in this PR could be fed into the README.md. Otherwise, I left a couple of comments.

spec.yaml Outdated Show resolved Hide resolved
spec.yaml Outdated Show resolved Hide resolved
This patch adds information about MFA to the readme-file and renames
`mfa-enforcing` to `mfa-capable`. The respons is simplified from a
boolean response on the endpoint to an empty HTTP 200 OK response.

Version is reset to 1.1.0
@mickenordin mickenordin changed the title V1.2.0 proposal, Add MFA capability and permission Add MFA capability and permission Jan 13, 2024
@mickenordin mickenordin marked this pull request as ready for review January 13, 2024 19:05
Copy link
Member

@glpatcern glpatcern left a comment

Choose a reason for hiding this comment

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

Looks good! @smesterheide and/or @michielbdejong any further thoughts?

@glpatcern glpatcern self-requested a review March 6, 2024 13:45
Copy link
Contributor

@michielbdejong michielbdejong left a comment

Choose a reason for hiding this comment

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

We should maybe have a more generic way to discover capabilities.
In practice, current implementations rely on /ocm-provider I think.
Did we want to move away from that?
In any case, shouldn't we use something like /.well-known/ocm-configuration?

@glpatcern
Copy link
Member

glpatcern commented Mar 8, 2024

I remember that /.well-known was/is problematic for Nextcloud, and they explicitly asked to stick to the current /ocm-provider for discovery. Perhaps worth checking this again now...

@mickenordin
Copy link
Contributor Author

Does it look like you expected now @glpatcern? I am not sure I did the right thing when merging your changes...

@glpatcern
Copy link
Member

Yes, I think this is OK (modulo one spurious thing easy to fix). For the discovery endpoint, that's something that was discussed in #37 (comment) and at the time it was decided NOT to go to .well-known. Time to rediscuss it, but outside of this PR.

spec.yaml Outdated Show resolved Hide resolved
spec.yaml Outdated Show resolved Hide resolved
mickenordin and others added 2 commits March 18, 2024 18:28
Co-authored-by: Giuseppe Lo Presti <giuseppe.lopresti@cern.ch>
Co-authored-by: Giuseppe Lo Presti <giuseppe.lopresti@cern.ch>
@glpatcern glpatcern merged commit af29d75 into cs3org:develop Mar 18, 2024
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