Skip to content
This repository has been archived by the owner on May 10, 2019. It is now read-only.

#49 Simple kubecfg.yaml endpoint security. #54

Closed
wants to merge 0 commits into from
Closed

#49 Simple kubecfg.yaml endpoint security. #54

wants to merge 0 commits into from

Conversation

joaovrodrigues
Copy link

@joaovrodrigues joaovrodrigues commented Nov 1, 2018

Added simple verification to the kubecfg.yaml endpoint, should show the 404 page when user not logged in and by doing so preventing partial credentials from being exposed publicly.

@codecov
Copy link

codecov bot commented Nov 1, 2018

Codecov Report

Merging #54 into master will decrease coverage by 0.38%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #54      +/-   ##
==========================================
- Coverage    39.6%   39.21%   -0.39%     
==========================================
  Files           1        1              
  Lines         202      204       +2     
==========================================
  Hits           80       80              
- Misses        116      118       +2     
  Partials        6        6
Impacted Files Coverage Δ
cmd/kuberos/kuberos.go 39.21% <0%> (-0.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7bc1e72...2f32f72. Read the comment docs.

Copy link
Owner

@negz negz left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this!

I've taken a first pass review. It seems like this PR needs a bit of cleanup before it can be merged; there's a few fmt statements that I assume were intended for debugging, as well as a bunch of spurious whitespace changes and permission changes on the shell scripts. Please clean these things up and I'll take another look.

extractor/oidc.go Outdated Show resolved Hide resolved
extractor/oidc.go Outdated Show resolved Hide resolved
extractor/oidc.go Outdated Show resolved Hide resolved
kuberos.go Outdated Show resolved Hide resolved
kuberos.go Outdated Show resolved Hide resolved
kuberos.go Outdated Show resolved Hide resolved
kuberos.go Outdated
if err := decoder.Decode(p, r.Form); err != nil {
http.Error(w, errors.Wrap(err, "cannot parse URL parameter").Error(), http.StatusBadRequest)
return
} else if len(p.Username) == 0 {
http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound)
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer to return HTTP 401 Unauthorized in this case.

@joaovrodrigues
Copy link
Author

joaovrodrigues commented Nov 5, 2018

So I accidentally added a commit from experimenting/exploring with Kubernetes, go-client and OIDC where I added/removed a bunch of log lines and code as I'm making my way through this tech stack. Removed that commit as it's not part of the scope of this PR.

Permission changes are a result from me moving away from macOS and tweaking with WSL for the scripts to work, also should not be in this commit.

I personally prefer to to have 404 in pages that shouldn't be accessible from the outside, and believe it's industry standard as well, but 401 is also a valid status code, changed it to that.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants