Add AES-CCM support #46

daenney · 2019-03-26T16:01:25Z

This is the first step towards completing #45.

daenney · 2019-03-26T16:07:55Z

So CI fails b/c of lint issues:

internal/crypto/ccm/ccm.go:68:13: captLocal: `L' should not be capitalized (gocritic)
func maxlen(L uint8, tagsize int) int {
            ^
internal/crypto/ccm/ccm.go:203:5: var errOpen is unused (unused)
var errOpen = errors.New("ccm: message authentication failed")
    ^
internal/crypto/ccm/ccm.go:213:25: should use make([]byte, int(c.M)) instead (gosimple)
	var tag = make([]byte, int(c.M), int(c.M))
	                       ^
internal/crypto/ccm/ccm.go:143:50: unnecessary conversion (unconvert)
				binary.BigEndian.PutUint64(block[2:i], uint64(n))

I can fix the first three, but I'm not so convinced about the last one. It's not strictly necessary I suppose but it does break with the symmetry going on in that block.

Sean-Der · 2019-03-26T18:57:32Z

Nice, this makes the next step a lot easier!

I am 100% with merging after

I would see if you can fix those lint issues
Would it be possible to add some basic tests?

It doesn't look like any ciphersuites use CCM and non-PSK right? that makes this a little harder to test, but not a big deal! It would just be great if we could confirm this all works before having to worry about PSK

daenney · 2019-03-27T07:10:00Z

So I have one lint error I don't want to fix:

internal/crypto/ccm/ccm.go:68:13: captLocal: `L' should not be capitalized (gocritic)
func maxlen(L uint8, tagsize int) int {

I agree with gocritic except that in this case the use of M and L directly related to the CCM RFC: https://tools.ietf.org/html/rfc3610. I think it's helpful to keep that way b/c it makes it easy to relate what these values are back to the spec.

daenney · 2019-03-27T08:21:17Z

That was a lot of CI wrangling 😅. The commit linter is overly paranoid from my pov, commits with just a subject and no body should be allowed; sometimes there's just not that much to say.

daenney · 2019-03-27T08:27:02Z

@Sean-Der So on the testing thing... I can add a few for simpler functions but ultimately I'd like to have some inputs/outputs to test Seal and Open with.

At the end of RFC 3610, in section 8, are a number of test vectors: https://tools.ietf.org/html/rfc3610. Could you suggest how to transform one into a test, and then I can add the rest?

backkem · 2019-03-27T15:24:28Z

@daenney You can use the following syntax in Go to represent hex:

https://github.com/pions/sctp/blob/5179f77cf05608bea6fd33c66a2820662858b089/param_outgoing_reset_request_test.go#L10-L11

For testing we usually do this type of table testing setup:
https://github.com/pions/sctp/blob/5179f77cf05608bea6fd33c66a2820662858b089/chunk_forward_tsn_test.go#L11

You would have the binary input and you can Seal/Open and verify if you get back where you started or verify against an expected result.

Sean-Der · 2019-03-30T09:02:39Z

Hey @daenney

Those test vectors in the RFC look perfect! If you get a chance I would add them in, and this is a merge from me!

After that I can help with getting the two DTLS implementations talking to each other (hack it up ugly in a branch) and we can just go from there, make it configurable and doing the right thing.

thanks!

daenney · 2019-03-30T12:21:51Z

@Sean-Der @backkem I've added a test based on the first vector in the RFC but it fails. I'm going to need someone to take a look at this and tell me what I've misunderstood b/c I'm stumped. I'm not going to be able to make any progress on this anymore without some help. I took a look at https://github.com/ircmaxell/quality-checker/blob/master/tmp/gh_18/PHP-PasswordLib-master/test/Data/Vectors/ccm-RFC3610.test-vectors but that's not helping me understand what I've missed either.

This is the first step towards completing #45.

This is necessary in order to be able to define exclude-rules in the configuration

Though in general gocritic is correct, the usage of L relates directly to the CCM spec. Keeping it like this makes it easier to understand what the code is doing when reading the spec.

This also happens in stdlib's crypto/aes/aes_gcm in the Seal method so we should allow it too.

As mandated by the commit linter

daenney · 2019-03-30T15:40:10Z

Thanks @backkem for pointing out what was wrong with the tests on Slack. All fixed and good to go!

The vectors run out of order b/c of the t.Parallel() but here we are:

--- PASS: TestRFC3610Vectors (0.01s)
    --- PASS: TestRFC3610Vectors/packet_vector_#24 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#24/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#24/open (0.00s)
    --- PASS: TestRFC3610Vectors/packet_vector_#5 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#5/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#5/open (0.00s)
    --- PASS: TestRFC3610Vectors/packet_vector_#12 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#12/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#12/open (0.00s)
    --- PASS: TestRFC3610Vectors/packet_vector_#13 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#13/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#13/open (0.00s)
    --- PASS: TestRFC3610Vectors/packet_vector_#10 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#10/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#10/open (0.00s)
    --- PASS: TestRFC3610Vectors/packet_vector_#9 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#9/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#9/open (0.00s)
    --- PASS: TestRFC3610Vectors/packet_vector_#11 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#11/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#11/open (0.00s)
    --- PASS: TestRFC3610Vectors/packet_vector_#8 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#8/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#8/open (0.00s)
    --- PASS: TestRFC3610Vectors/packet_vector_#7 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#7/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#7/open (0.00s)
    --- PASS: TestRFC3610Vectors/packet_vector_#6 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#6/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#6/open (0.00s)
    --- PASS: TestRFC3610Vectors/packet_vector_#4 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#4/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#4/open (0.00s)
    --- PASS: TestRFC3610Vectors/packet_vector_#3 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#3/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#3/open (0.00s)
    --- PASS: TestRFC3610Vectors/packet_vector_#2 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#2/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#2/open (0.00s)
    --- PASS: TestRFC3610Vectors/packet_vector_#23 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#23/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#23/open (0.00s)
    --- PASS: TestRFC3610Vectors/packet_vector_#20 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#20/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#20/open (0.00s)
    --- PASS: TestRFC3610Vectors/packet_vector_#22 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#22/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#22/open (0.00s)
    --- PASS: TestRFC3610Vectors/packet_vector_#19 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#19/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#19/open (0.00s)
    --- PASS: TestRFC3610Vectors/packet_vector_#18 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#18/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#18/open (0.00s)
    --- PASS: TestRFC3610Vectors/packet_vector_#21 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#21/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#21/open (0.00s)
    --- PASS: TestRFC3610Vectors/packet_vector_#16 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#16/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#16/open (0.00s)
    --- PASS: TestRFC3610Vectors/packet_vector_#17 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#17/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#17/open (0.00s)
    --- PASS: TestRFC3610Vectors/packet_vector_#15 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#15/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#15/open (0.00s)
    --- PASS: TestRFC3610Vectors/packet_vector_#1 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#1/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#1/open (0.00s)
    --- PASS: TestRFC3610Vectors/packet_vector_#14 (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#14/seal (0.00s)
        --- PASS: TestRFC3610Vectors/packet_vector_#14/open (0.00s)

This adds a number of tests based on the 24 vectors defined in RFC 3610 to ensure the implementation is correct.

Sean-Der · 2019-03-30T19:15:26Z

Hey @daenney

Everything LGTM! You should have a Rebase and merge button on this page now, lack of coverage is a soft fail (only failing travis-ci prevents a merge)

Really nice work on getting this done, onto the next step :)

daenney changed the title ~~Add AES-CCM support for IoT use-cases~~ Add AES-CCM support Mar 26, 2019

backkem approved these changes Mar 26, 2019

View reviewed changes

daenney requested a review from Sean-Der March 27, 2019 08:30

daenney added 5 commits March 30, 2019 14:52

Add AES-CCM support for IoT use-cases

0913b28

This is the first step towards completing #45.

CI: Upgrade golang-ci to 1.15

28d4a9b

This is necessary in order to be able to define exclude-rules in the configuration

CI: Disable captLocal check internal/crypto/ccm

0eafe53

Though in general gocritic is correct, the usage of L relates directly to the CCM spec. Keeping it like this makes it easier to understand what the code is doing when reading the spec.

CI: crypto/ccm is allowed to panic

4380100

This also happens in stdlib's crypto/aes/aes_gcm in the Seal method so we should allow it too.

CI: Add myself to README

228dd55

As mandated by the commit linter

CCM: Add tests based on RFC 3610 vectors

82006ce

This adds a number of tests based on the 24 vectors defined in RFC 3610 to ensure the implementation is correct.

daenney merged commit e63b39a into pion:master Mar 31, 2019

daenney deleted the aes-ccm branch March 31, 2019 09:38

daenney mentioned this pull request Mar 31, 2019

Support TLS_PSK_WITH_AES_128_CCM_8 for IoT usecases #45

Closed

3 tasks

daenney mentioned this pull request Jul 4, 2019

Release 1.4.0 #80

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AES-CCM support #46

Add AES-CCM support #46

daenney commented Mar 26, 2019

daenney commented Mar 26, 2019 •

edited

Loading

Sean-Der commented Mar 26, 2019

daenney commented Mar 27, 2019 •

edited

Loading

daenney commented Mar 27, 2019

daenney commented Mar 27, 2019

backkem commented Mar 27, 2019 •

edited

Loading

Sean-Der commented Mar 30, 2019

daenney commented Mar 30, 2019 •

edited

Loading

daenney commented Mar 30, 2019

Sean-Der commented Mar 30, 2019 •

edited

Loading

Add AES-CCM support #46

Add AES-CCM support #46

Conversation

daenney commented Mar 26, 2019

daenney commented Mar 26, 2019 • edited Loading

Sean-Der commented Mar 26, 2019

daenney commented Mar 27, 2019 • edited Loading

daenney commented Mar 27, 2019

daenney commented Mar 27, 2019

backkem commented Mar 27, 2019 • edited Loading

Sean-Der commented Mar 30, 2019

daenney commented Mar 30, 2019 • edited Loading

daenney commented Mar 30, 2019

Sean-Der commented Mar 30, 2019 • edited Loading

daenney commented Mar 26, 2019 •

edited

Loading

daenney commented Mar 27, 2019 •

edited

Loading

backkem commented Mar 27, 2019 •

edited

Loading

daenney commented Mar 30, 2019 •

edited

Loading

Sean-Der commented Mar 30, 2019 •

edited

Loading