pkg/aead: add explicit data race and locking to resolve #77

jphines · 2018-10-01T13:35:40Z

This adds an explicit concurrency test case for aead/miscreant to test for potential data races in the underlying cipher implementation.

katzdm

Is there a unit test we could construct that would repro the data race when run with go test -race (and fail against HEAD), which we could then verify is fixed with this patch?

katzdm · 2018-10-01T14:02:35Z

internal/pkg/aead/aead.go

@@ -61,6 +67,11 @@ func (c *MiscreantCipher) Encrypt(plaintext []byte) (joined []byte, err error) {

 // Decrypt a value using AES-CMAC-SIV
 func (c *MiscreantCipher) Decrypt(joined []byte) ([]byte, error) {
+	aead, err := miscreant.NewAEAD(algorithmType, c.secret, miscreantNonceSize)


It's worth pointing out that this seems to be a somewhat nontrivial initializer, which performs some block cipher encryption to derive its initial state, on each call.
https://github.com/miscreant/miscreant-go/blob/master/cmac/cmac.go#L45

My assumption is that sso_proxy is not particularly compute-bounded, but it could be interesting to throw this up in stage, and using Datadog to verify that we don't see an undue latency cost from these initializations.

I suppose instead of instantiating new objects we could just lock around a single cipher?

I wonder which would be more performant.

jphines · 2018-10-01T16:45:04Z

@katzdm Do you mean something like https://circleci.com/gh/buzzfeed/sso/463 which shows the data race?

katzdm · 2018-10-01T16:46:37Z

internal/pkg/aead/aead.go

@@ -25,17 +25,18 @@ type Cipher interface {
 // Using an AEAD is a cipher providing authenticated encryption with associated data.
 // For a description of the methodology, see https://en.wikipedia.org/wiki/Authenticated_encryption
 type MiscreantCipher struct {
-	aead cipher.AEAD
+	secret []byte
 }

 // NewMiscreantCipher returns a new AES Cipher for encrypting values
 func NewMiscreantCipher(secret []byte) (*MiscreantCipher, error) {


This is a somewhat minor point, but it feels surprising for this type to be named MiscreantCipher, if the object in question (the Miscreant cipher) is only instantiated at Encrypt/Decrypt-time. Maybe it would be better if we keep the cipher.AEAD object owned by the MiscreantCipher class, and instead initialize a new MiscreantCipher during Marshal/Unmarsal. Might make it easier to reason about the types, in the future.

What do you think?

I'm okay with changing the type name, but I think whatever we want to expose, we want to be go-routine safe. I would argue that this should include making both Encrypt and Decrypt go-routine safe which currently necessitates the instantiation of new objects.

I think just creating new objects in the Unmarshal and Marshal such that those are the only two methods that would be go-routine safe isn't sufficient for what we want to expose from this API.

FWIW the MiscreantCipher is an implementation of the Cipher interface (which all might be poorly named) and has Unmarshal and Marshal as well as Encrypt and Decrypt functions. This probably could be structured differently but we'd just need to reflect those changes in the MockCipher

katzdm · 2018-10-01T17:59:27Z

internal/pkg/aead/aead_test.go

@@ -89,3 +93,72 @@ func TestMarshalAndUnmarshalStruct(t *testing.T) {
 		t.Fatalf("expected structs to be equal")
 	}
 }
+
+func TestCipherDataRace(t *testing.T) {


Consider annotating with a comment pointing to a GitHub issue describing the problem. Six months from now, we'll all forget why we added this test :-)

katzdm · 2018-10-01T18:12:17Z

internal/pkg/aead/aead_test.go

+// TestCipherDataRace exercises a simple concurrency test for the MicscreantCipher.
+// In https://github.com/buzzfeed/sso/pull/75 we investigated why, on random occasion,
+// unmarshalling session states would fail, triggering users to get kicked out of
+// authenticated states. We narrowed our suspicious to a data race that we uncovered


nit: s/suspicious/suspicions

Also, I don't think we should describe the miscreant library as "having a data race"; the library just doesn't make any attempt at thread-safety. It might be more accurate to phrase the situation as something like: "We eventually uncovered that sso implicitly assumed the underlying miscreant APIs to be thread-safe; in truth, they are only thread-compatible and require external synchronization."

shrayolacrayon · 2018-10-01T18:45:50Z

this LGTM, lets squash + rebase and merge?

jphines · 2018-10-01T18:47:11Z

@shrayolacrayon squashed into two commits + rebased.

loganmeetsworld added the in progress label Oct 1, 2018

katzdm reviewed Oct 1, 2018

View reviewed changes

jphines force-pushed the jhines-data-race branch from 502ca31 to 48d267d Compare October 1, 2018 17:40

katzdm reviewed Oct 1, 2018

View reviewed changes

katzdm previously approved these changes Oct 1, 2018

View reviewed changes

jphines changed the title ~~[wip] pkg/aead: add explicit data race test~~ pkg/aead: add explicit data race and locking to resolve data race Oct 1, 2018

jphines changed the title ~~pkg/aead: add explicit data race and locking to resolve data race~~ pkg/aead: add explicit data race and locking to resolve Oct 1, 2018

jphines dismissed katzdm’s stale review via 4c525f4 October 1, 2018 18:05

katzdm reviewed Oct 1, 2018

View reviewed changes

jphines force-pushed the jhines-data-race branch 2 times, most recently from bbc1525 to b1beac2 Compare October 1, 2018 18:16

jphines added 2 commits October 1, 2018 14:46

pkg/aead: add explicit data race test

15c0fd9

aead: use simple mutex instead

63af567

jphines force-pushed the jhines-data-race branch from b1beac2 to 63af567 Compare October 1, 2018 18:46

shrayolacrayon approved these changes Oct 1, 2018

View reviewed changes

katzdm approved these changes Oct 1, 2018

View reviewed changes

jphines merged commit c2a68c1 into master Oct 1, 2018

jphines deleted the jhines-data-race branch October 1, 2018 18:57

loganmeetsworld removed the in progress label Oct 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/aead: add explicit data race and locking to resolve #77

pkg/aead: add explicit data race and locking to resolve #77

jphines commented Oct 1, 2018

katzdm left a comment

katzdm Oct 1, 2018

jphines Oct 1, 2018

jphines commented Oct 1, 2018 •

edited

Loading

katzdm Oct 1, 2018

jphines Oct 1, 2018

jphines Oct 1, 2018

shrayolacrayon Oct 1, 2018

katzdm Oct 1, 2018

jphines Oct 1, 2018

katzdm Oct 1, 2018

shrayolacrayon commented Oct 1, 2018

jphines commented Oct 1, 2018

pkg/aead: add explicit data race and locking to resolve #77

pkg/aead: add explicit data race and locking to resolve #77

Conversation

jphines commented Oct 1, 2018

katzdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jphines commented Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrayolacrayon commented Oct 1, 2018

jphines commented Oct 1, 2018

jphines commented Oct 1, 2018 •

edited

Loading