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

Improvements regarding signed URLs #380

Merged
merged 2 commits into from
Apr 15, 2021
Merged

Improvements regarding signed URLs #380

merged 2 commits into from
Apr 15, 2021

Conversation

m-mohr
Copy link
Member

@m-mohr m-mohr commented Apr 14, 2021

Solves issue #379

@m-mohr m-mohr added this to the 1.1.0 milestone Apr 14, 2021
@m-mohr m-mohr requested a review from soxofaan April 14, 2021 12:57
@m-mohr m-mohr linked an issue Apr 14, 2021 that may be closed by this pull request
CHANGELOG.md Outdated Show resolved Hide resolved
errors.json Outdated Show resolved Hide resolved
errors.json Outdated Show resolved Hide resolved
URL signing is a way to protect files from unauthorized access with a
key in the URL instead of HTTP header based authorization. The URL
signing key is similar to a password and its inclusion in the URL allows
to download files using simple GET requests supported by a wide range of
programs, e.g. web browsers or download managers. Back-ends are
responsible to generate the URL signing keys and to manage their
appropriate expiration. The back-end MAY indicate an expiration time by
setting the `expires` property.

setting the `expires` property in the reponse. Requesting this endpoint
Copy link
Member

Choose a reason for hiding this comment

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

I would add a recommendation that the expiry time should be order of minutes/hours "to give clients enough time to download all assets, while still mitigating the security risk.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm... I'm not sure how high the security risk is actually. Google uses signed URLs for Docs, Spreadsheets etc. and they seem to not expire at all.

Copy link
Member

Choose a reason for hiding this comment

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

a difference is that the google doc user can stop sharing a doc (invalidate the signed url)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is what issue #341 has been opened for.

openapi.yaml Outdated Show resolved Hide resolved
@soxofaan
Copy link
Member

After pondering some more on this, some thoughts:

  • shouldn't we be more enforcing about the expiration time for security reasons (e.g. at least explicitly RECOMMEND it)? The current statement "Back-ends are responsible ..." is weaker than a recommendation I think.
  • Would it make sense to let the user choose whether they want public signed URLs that can expire or URLs protected with bearer token auth (that can not expire)? The expiry problem is mitigated by the renewal mechanism, but security minded users might want to avoid publicly available URLs that they can not invalidate themself.

I know, the tin foil hat factor of these points is quite high, and it's probably not urgent to tackle these the moment, but it might become a problem in the future.

@m-mohr
Copy link
Member Author

m-mohr commented Apr 14, 2021

  • shouldn't we be more enforcing about the expiration time for security reasons (e.g. at least explicitly RECOMMEND it)? The current statement "Back-ends are responsible ..." is weaker than a recommendation I think.

As also stated above, I'm not sure how high the security risk is actually, so I'm not sure this is actually required.

  • Would it make sense to let the user choose whether they want public signed URLs that can expire or URLs protected with bearer token auth (that can not expire)?

I assume that could make sense, but that would need to be tracked in a new issue for 2.0 (breaking for clients).

The expiry problem is mitigated by the renewal mechanism

The wording we add here doesn't necessarily mean that previous signed URLs get invalid, it just means expired links will be renewed. Revoking signed URLs is a different issue: #341 We may decide that re-requesting this endpoint also invalidates previously generated signed URLs, but currently that's not the intention yet.

@soxofaan
Copy link
Member

I didn't know about #341 , that should indeed tackle most of the concerns raised here.

@m-mohr
Copy link
Member Author

m-mohr commented Apr 15, 2021

@soxofaan It seems this is important for you? I can certainly move the milestone up for #341 and make a proposal for API 1.1 in the next days.

@soxofaan
Copy link
Member

Well, to give some background: the current implementation that @laxiwuka is working on for the VITO backend does not allow invalidation (there is no storage, just an added hash in the URL based on a secret to check that the URL hasn't been tampered with).
The only tool to improve security is using shorter expiration times (changing the secret would invalidate all URLs for all users).

#341 requires more work (because of persistent storage involved) than other alternatives (that do not require persistent storage or state) I am exploring here:

  • push the spec to make expiration times as short as possible
  • make it possible to opt-in for bearer-token based urls
  • allow user or client to set expiration time in some way

@m-mohr
Copy link
Member Author

m-mohr commented Apr 15, 2021

Thanks for the details, @soxofaan. It sounds like this is not your long-term solution though so I'll merge this and have opened PR #381 with a proposal to revoke signed URLs.

@m-mohr m-mohr merged commit 3340868 into draft Apr 15, 2021
@m-mohr m-mohr deleted the batch-job-expiry branch April 15, 2021 11:20
@soxofaan
Copy link
Member

Yes, good to merge this already.
Another follow up about toggling between signed urls and bearer token download urls: #382

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.

error code for expired file access
2 participants