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 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 added enhancement New feature or request in progress labels Oct 31, 2018
@katzdm katzdm force-pushed the request-signing branch 3 times, most recently from f0942e8 to 4a86464 Compare October 31, 2018 15:41
@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 Compare November 12, 2018 16:26
// used to sign each request.
func (p *OAuthProxy) Certs(rw http.ResponseWriter, _ *http.Request) {
rw.WriteHeader(http.StatusOK)
fmt.Fprint(rw, p.publicCertsJSON)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, good suggestion. Thanks!

// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 Compare November 19, 2018 19:24
@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

one nit but otherwise this looks great!

@katzdm katzdm force-pushed the request-signing branch 5 times, most recently from 4deead6 to 443a712 Compare November 27, 2018 16:10
@katzdm katzdm merged commit cbae634 into master Nov 27, 2018
@katzdm katzdm deleted the request-signing branch December 3, 2018 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants