X Tutup
The Wayback Machine - https://web.archive.org/web/20200927054625/https://github.com/buzzfeed/sso/pull/106
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

sso-proxy: sign upstream requests with a verifiable digital signature #106

Merged
merged 1 commit into from Nov 27, 2018

Conversation

@katzdm
Copy link
Contributor

katzdm commented Oct 31, 2018

Enable the signing of upstream requests using a digital signature, instead of requiring a shared secret for each upstream service. Work in progress.

@katzdm katzdm force-pushed the request-signing branch 3 times, most recently from f0942e8 to 4a86464 Oct 31, 2018
@jphines
Copy link
Contributor

jphines commented Oct 31, 2018

Resolves #16 and potentially resolves #45 dependent on the specific implementation

@katzdm
Copy link
Contributor Author

katzdm commented Nov 6, 2018

Resolves #16 and potentially resolves #45 dependent on the specific implementation

I'm looking at addressing these issues separately. This PR will (soon) address #16; issue #45 is forthcoming.

@katzdm katzdm force-pushed the request-signing branch 10 times, most recently from 3dd156b to 1a37db1 Nov 7, 2018
@katzdm katzdm requested review from shrayolacrayon and jphines Nov 12, 2018
@katzdm katzdm removed the in progress label Nov 12, 2018
// used to sign each request.
func (p *OAuthProxy) Certs(rw http.ResponseWriter, _ *http.Request) {
rw.WriteHeader(http.StatusOK)
fmt.Fprint(rw, p.publicCertsJSON)

This comment has been minimized.

@jphines

jphines Nov 16, 2018 Contributor

We don't need to convert this to a string to use it here, nor do we have to call WriteHeader, we could just:

rw.Write(p.RawPublicCerts)

Which will default to 200 and write our string as bytes.

This comment has been minimized.

@katzdm

katzdm Nov 19, 2018 Author Contributor

Nice, good suggestion. Thanks!

internal/proxy/oauthproxy.go Outdated Show resolved Hide resolved
internal/proxy/oauthproxy.go Outdated Show resolved Hide resolved
// hash value received through the `Octoboi-Signature` of the request.
//
// Any requests failing this check should be considered tampered with, and rejected.
func (signer RequestSigner) Sign(req *http.Request) error {

This comment has been minimized.

@jphines

jphines Nov 16, 2018 Contributor

Since this isn't a function pointer, does this create a new object when called?

Is that why we don't have to worry about concurrent and/or racey writes to the hasher object below, since it's a new object on each Sign invocation?

This comment has been minimized.

@jphines

jphines Nov 16, 2018 Contributor

Or should this be a pointer handler and we do have worry about concurrent and racey writes to the hasher?

This comment has been minimized.

@katzdm

katzdm Nov 19, 2018 Author Contributor

Right, this creates a new object, which I believe ought to avoid racey writes to the hasher and signingKey fields.

@katzdm katzdm force-pushed the request-signing branch 2 times, most recently from 993e32d to e5f71c5 Nov 19, 2018
@jphines
Copy link
Contributor

jphines commented Nov 19, 2018

LGTM

@mreiferson mreiferson changed the title Sign upstream requests with a verifiable digital signature. sso-proxy: sign upstream requests with a verifiable digital signature Nov 26, 2018
@shrayolacrayon
Copy link
Contributor

shrayolacrayon commented Nov 26, 2018

one nit but otherwise this looks great!

@katzdm katzdm force-pushed the request-signing branch 5 times, most recently from 4deead6 to 443a712 Nov 26, 2018
@katzdm katzdm force-pushed the request-signing branch from 443a712 to 93ab018 Nov 27, 2018
@katzdm katzdm merged commit cbae634 into master Nov 27, 2018
1 check passed
1 check passed
ci/circleci Your tests passed on CircleCI!
Details
@katzdm katzdm deleted the request-signing branch Dec 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.
X Tutup