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

fix: allow requests to /api/* without authentication #2455

Merged

Conversation

vincentgna
Copy link
Contributor

@vincentgna vincentgna commented Aug 18, 2022

Problem

If WebAuthentication is enabled, API Endpoints can't be hit without Basic Auth on top of API Secret.

Not sure if this is by design, but I'd assume API Tokens should be decoupled from the WebAuthentication?

Expected

curl -vH "X-Atlantis-Token: ${ATLANTIS_API_SECRET}" \
  -H "Accept: application/json" \
  -H "Content-Type: application/json"  \
  http://localhost:4141/api/plan \
  -d '{ "repository": "...","ref":"...","type":"...","projects":["..."]}'
 > OK

Actual

curl -vH "X-Atlantis-Token: ${ATLANTIS_API_SECRET}" \
  -H "Accept: application/json" \
  -H "Content-Type: application/json"  \
  http://localhost:4141/api/plan \
  -d '{ "repository": "...","ref":"...","type":"...","projects":["..."]}'
 > Unauthorized

Work Around:

curl -vH "X-Atlantis-Token: ${ATLANTIS_API_SECRET}" \
  -H "Authorization: Basic ${ATLANTIS_BASIC_AUTH_BASE64}" 
  -H "Accept: application/json" \
  -H "Content-Type: application/json"  \
  http://localhost:4141/api/plan \
  -d '{ "repository": "...","ref":"...","type":"...","projects":["..."]}'
> OK

Related to:

@vincentgna vincentgna requested a review from a team as a code owner August 18, 2022 03:40
@vincentgna
Copy link
Contributor Author

I assumed the APIController was handling all routes prefixed with /api/ (which is the case today)

atlantis/server/server.go

Lines 829 to 830 in b15b5dc

s.Router.HandleFunc("/api/plan", s.APIController.Plan).Methods("POST")
s.Router.HandleFunc("/api/apply", s.APIController.Apply).Methods("POST")

but is this a safe assumption, or should we explicitly only allow api/plan and /api/apply for now? I understand the API Token authentication right now is quite KISS

@jamengual
Copy link
Contributor

I think this was on purpose but it was build a while ago what do you think @lilincmu

@jamengual jamengual added waiting-on-review Waiting for a review from a maintainer api-endpoints Adding API endpoints to Atlantis labels Aug 18, 2022
@vincentgna
Copy link
Contributor Author

there are no docs, so it was a bit frustrating for me trying this out :) - which is why I also linked it into the Issue tracking the feature for anyone else hitting the same problem

@jamengual
Copy link
Contributor

jamengual commented Aug 18, 2022 via email

@vincentgna
Copy link
Contributor Author

vincentgna commented Aug 18, 2022

PR to update the docs is always welcome.

I realise that, and I thought to do it in a separate PR once I got to play and understand this PoC a bit better, I went through all the discussion on the feature and I see no mention of requiring both basic auth and api token, but let's wait for @lilincmu advice on this.

but thanks for the suggestion @jamengual !

@lilincmu
Copy link
Contributor

The implementation of #997 didn't take WebAuthentication into consideration, since its first draft was created before the PR of WebAuthentication. I can see the name api-secret could be confusing to users, who now might assume they only need to choose either authentication, the X-Atlantis-Token or Basic Auth, in order to make successful API requests, while in reality they have to use X-Atlantis-Token anyways if they want to make plan/apply API requests.

I think it makes sense to bypass the basic auth for plan/apply for now, since the requests will be "authenticated" by X-Atlantis-Token eventually. However, I don't know if any user would expect the opposite, i.e., when basic auth is enabled, any requests made to Atlantis, including plan/apply, should be authenticated by basic auth.

@vincentgna Maybe we can let this PR sit for a while and see what the community thinks? If no one responds by next week I can merge it in.

@lilincmu lilincmu removed the waiting-on-review Waiting for a review from a maintainer label Aug 19, 2022
@jamengual jamengual added help wanted Good feature for contributors needs discussion Large change that needs review from community/maintainers labels Aug 22, 2022
@vincentgna
Copy link
Contributor Author

FYI, moving authentication (SSO) to reverse proxy kind of resolves this issue for our use case (it was a planned to do)

@jamengual jamengual merged commit 99e1f10 into runatlantis:master Sep 8, 2022
@nitrocode nitrocode mentioned this pull request Oct 25, 2022
krrrr38 pushed a commit to krrrr38/atlantis that referenced this pull request Dec 16, 2022
@nitrocode nitrocode added this to the v0.19.9 milestone Jan 13, 2023
@vincentgna vincentgna deleted the exclude-api-controller-webauth branch September 19, 2023 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-endpoints Adding API endpoints to Atlantis help wanted Good feature for contributors needs discussion Large change that needs review from community/maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants