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

Describe authentication methods in TAMS API #113

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

samdbmg
Copy link
Member

@samdbmg samdbmg commented Feb 12, 2025

Details

  • Adds an ADR describing possible authentication methods
  • Proposes the use of Bearer tokens, tokens in URLs and HTTP Basic Auth
  • Makes changes to the spec to call this out in more detail

Note: I'd like to discuss the merits of removing "No auth" as an option in the spec: it's a rather convenient security risk, but my initial thinking is it shouldn't be explicitly allowed! I'm also unsure whether it should be a breaking change - again, for discussion.

There will be a followup PR on authorization

Jira Issue (if relevant)

Jira URL: https://jira.dev.bbc.co.uk/browse/CLOUDFIT-3532

Related PRs

N/A

Submitter PR Checks

(tick as appropriate)

  • PR completes task/fixes bug
  • API version has been incremented if necessary
  • ADR status has been updated, and ADR implementation has been recorded
  • Documentation updated (README, etc.)
  • PR added to Jira Issue (if relevant)
  • Follow-up stories added to Jira

Reviewer PR Checks

(tick as appropriate)

  • PR completes task/fixes bug
  • Design makes sense, and fits with our current code base
  • Code is easy to follow
  • PR size is sensible
  • Commit history is sensible and tidy

Info on PRs

The checks above are guidelines. They don't all have to be ticked, but they should all have been considered.

@samdbmg samdbmg force-pushed the sammg-authentication-methods branch from ac4d301 to 0f148f0 Compare February 12, 2025 10:01
@samdbmg samdbmg marked this pull request as ready for review February 12, 2025 10:01
@samdbmg samdbmg requested a review from a team as a code owner February 12, 2025 10:01
Adjusts the specification to call out the authentication methods in use
by the API in more detail.
Removes "no auth" as an option (breaking change)
Adds a few words about security to the README

sem-ver: api-break
@samdbmg samdbmg force-pushed the sammg-authentication-methods branch from 0f148f0 to 0966d6b Compare February 12, 2025 14:37
Copy link
Contributor

@philipnbbc philipnbbc left a comment

Choose a reason for hiding this comment

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

A good read and I generally agree. I did have a question inline about passing the token using a header instead and raised the issue of paging responses including the token. Otherwise LGTM.

url_token_auth:
type: apiKey
name: access_token
in: query
Copy link
Contributor

Choose a reason for hiding this comment

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

I wrote the comment below before reading the ADR. Supporting a presigned URL type approach makes sense but some of the potential issues described here still apply.

Not sure myself, but should in: header also be supported or used instead?

A counter argument for in: header is that a query parameter is easier when using a web browser and https://username:password@... works in some browsers for basic auth for example.

However, is it better to pass tokens using headers following the OAuth 2.0 recommendations?

Thinking about the next segments paging URL. Would it be acceptable to pass the token back in the response's Location header or would you expect it to not be included (similarly for basic auth)? I'm guessing any auth sitting in front of the TAMS core would likely strip that information anyway before the TAMS core sees it. Would it be less unexpected if the token was always inserted as a header by the client rather than as a query parameter?

Copy link
Member Author

Choose a reason for hiding this comment

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

That ease is what pushed me towards in: query (because then you get a single copy-pastable string that works), but it isn't ideal for the reasons in RFC 6750.

But that does create some rather complicated logic (even at the basic level of having to read it out of the first request and then re-attach it, so whatever you do the client has some work to do).
Presenting it in a header offers an interesting other opportunity too: we could define a mechanism where sometimes it comes back in the response headers, as a way to rotate the token. Not sure how much prior art there is for something like that though (maybe it's too much magic?)

I don't have a particularly strong opinion on this, but this is definitely the newest/most contentious bit of the proposal, so maybe a discussion to have with the wider group

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.

2 participants