Conversation
This will replace boring crypto at some point. We should modify our protocol a bit and instead change to NewGCMWithRandomNonce.
| cd .github/workflows/smoke/ && NAME="smoke-p256" CURVE="P256" ./build.sh | ||
| cd .github/workflows/smoke/ && NAME="smoke-p256" ./smoke.sh | ||
| cd .github/workflows/smoke/ && $(GOENV) ./build.sh | ||
| cd .github/workflows/smoke/ && $(GOENV)./smoke.sh |
There was a problem hiding this comment.
Missing a space after $(GOENV)
|
|
||
| type cipherAEADDanger interface { | ||
| EncryptDanger(out, ad, plaintext []byte, n uint64, nb []byte) ([]byte, error) | ||
| DecryptDanger(out, ad, plaintext []byte, n uint64, nb []byte) ([]byte, error) |
There was a problem hiding this comment.
im confused, when you call this function it is still plaintext
| @$(MAKE) service ${.DEFAULT_GOAL} --no-print-directory | ||
| endif | ||
|
|
||
| fips140: |
There was a problem hiding this comment.
boringcrypto outputs with a suffix on the folder like: build/linux-amd64-boringcrypto/nebula
fips140 outputs to a colliding path with non fips140: build/linux-amd64/nebula, seems like a good idea to push this into build/linux-amd64-fips140/nebula.
There was a problem hiding this comment.
good catch! let me rework this.
There was a problem hiding this comment.
added release-fips140 and bin-fips140 for this
| out = s.c.Seal(out, nb, plaintext, ad) | ||
| //l.Debugf("Encryption: outlen: %d, nonce: %d, ad: %s, plainlen %d", len(out), n, ad, len(plaintext)) | ||
| return out, nil | ||
| switch ce := s.c.(type) { |
There was a problem hiding this comment.
Would be awesome to swap out the interface and avoid the type check for each packet.
|
|
||
| fips140: | ||
| @echo > $(NULL_FILE) | ||
| $(eval GOENV += GOFIPS140=v1.0.0) |
There was a problem hiding this comment.
We will also need to require go 1.26 and include this LDFLAGS: -X runtime.godebugDefault=fips140=only
Otherwise fips is enabled but all non fips crypto just keeps working. Will need to couple this with a change to assert fips140.Enforced() is true.
There was a problem hiding this comment.
added the default, but not enforcing and instead just logging the state of it. Considering the check.
There was a problem hiding this comment.
Implemented with a build tag fips140enforce
| // EncryptLockNeeded indicates if calls to Encrypt need a lock | ||
| // This is true for fips140 because the Seal function verifies that the | ||
| // nonce is strictly increasing. | ||
| const EncryptLockNeeded = true |
There was a problem hiding this comment.
The other part of -X runtime.godebugDefault=fips140=only is that we will want an init check to assert fips140.Enforced() here.
There was a problem hiding this comment.
Implemented with a build tag fips140enforce.
This is safer because it doesn't clober other defaults that are set by Go or the environment.
Why not?
| } | ||
|
|
||
| func (c *aeadGCMFIPS140Cipher) Seal(dst, nonce, plaintext, additionalData []byte) []byte { | ||
| if !c.ready { |
There was a problem hiding this comment.
can we do this as part of extractFIPSAEAD? It would avoid a branch & technically also avoids an opportunity to race (thought my understanding is this should always be called under a lock bc of how the fips nonce checker works
There was a problem hiding this comment.
I think we can as long as nothing in our code encrypts with a "0" nonce. I think this is true. Ill test it
There was a problem hiding this comment.
actually, the first WriteMessage that noise does is with nonce 0, so this is a bit tricky to optimize
| switch curve { | ||
| case cert.Curve_CURVE25519: | ||
| if fips140.Enforced() { | ||
| panic("pki: use of Curve25519 is not allowed in FIPS 140-only mode") |
There was a problem hiding this comment.
Thoughts on returning an error instead of throwing a panic?
There was a problem hiding this comment.
We throw a panic for the other algorithms when fips140.Enforced is turned on. Let me check.
There was a problem hiding this comment.
I'm happy with a panic as long as we can show it's not reachable from UDP
There was a problem hiding this comment.
ill change to return an error, that matches what stdlib does in my research
|
Do we want to check for and respect |
Add support for the "fips140" mode of Go:
You can build with
make fips140, see the README changes for more info.Some differences from the boringcrypto builds:
go:linkname crypto/tls.aeadAESGCMTLS13, which gives us the fips implementation for bothboringcryptoandfips140modes. This means we also no longer need-checklinkname=0fips140doesn't need CGO_ENABLED=1fips140.Enabled()is true. If you use themake release-fips140, we build with build tagfips140enforcewhich ensures the binary is running with fips140 enabled and that only P256 / AES-GCM is being used. If you don't want this enforce mode, you can build without the build tag.