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: fix request signer hash panic #274

Merged
merged 5 commits into from
Dec 10, 2019

Conversation

Jusshersmith
Copy link
Contributor

@Jusshersmith Jusshersmith commented Dec 5, 2019

Problem

Occasional panics (http: panic serving 172.17.0.16:49478: d.nx != 0) which seem to be caused by sso_proxy using the same hasher for multiple requests, creating a race condition between the hasher.Reset() and hasher.Write([]byte(repr)) calls.

Solution

Instead of storing a hash.Hash object within the RequestSigner, instead store a function which creates a new hash.Hash. This lets each request be signed using a different Hash.

@jphines
Copy link
Contributor

jphines commented Dec 5, 2019

🤦‍♂

@codecov
Copy link

codecov bot commented Dec 9, 2019

Codecov Report

Merging #274 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #274      +/-   ##
==========================================
- Coverage   62.21%   62.18%   -0.03%     
==========================================
  Files          54       54              
  Lines        4200     4197       -3     
==========================================
- Hits         2613     2610       -3     
  Misses       1399     1399              
  Partials      188      188
Impacted Files Coverage Δ
internal/proxy/request_signer.go 84.84% <100%> (-0.66%) ⬇️

@Jusshersmith
Copy link
Contributor Author

Rebased and removed go.mod and go.sum changes, now that #273 is merged.

@Jusshersmith Jusshersmith marked this pull request as ready for review December 9, 2019 17:02
@Jusshersmith Jusshersmith added the bug Something isn't working label Dec 9, 2019
@Jusshersmith Jusshersmith self-assigned this Dec 9, 2019
@Jusshersmith
Copy link
Contributor Author

I think I've said the word 'hash' a few too many times now 🤔

@Jusshersmith Jusshersmith merged commit cb5e207 into master Dec 10, 2019
@Jusshersmith Jusshersmith deleted the jusshersmith-fix-hash-panic branch December 10, 2019 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants