Skip to content

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

🤦‍♂

@Jusshersmith Jusshersmith force-pushed the jusshersmith-fix-hash-panic branch from e234342 to 2384f7e Compare December 9, 2019 16:57
@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