FAQ
Even though it's trivial, a test or two would be nice.

The deps_test changes are fine.



https://codereview.appspot.com/6632044/diff/2002/src/pkg/crypto/hmac/hmac.go
File src/pkg/crypto/hmac/hmac.go (right):

https://codereview.appspot.com/6632044/diff/2002/src/pkg/crypto/hmac/hmac.go#newcode10
src/pkg/crypto/hmac/hmac.go:10: Receivers should be careful to use
Verify to compare MACs in order to avoid
new paragraph here (insert blank line above this one)

https://codereview.appspot.com/6632044/diff/2002/src/pkg/crypto/hmac/hmac.go#newcode95
src/pkg/crypto/hmac/hmac.go:95: // Verify compares two MACs for equality
without leaking timing information.
I'd prefer to use a name that is more clearly boolean. Also the
variables could be named to make clearer that the arguments are the
MACs.

func Equal(mac1, mac2 []byte) bool
?

On a related note, I was expecting Verify to be more of a helper. If you
want to make it dead simple to use correctly, I wonder if you should
add:

func Sign(h func() hash.Hash, key, msg []byte) (mac []byte)
func Verify(h func() hash.Hash, key, msg, mac []byte) error

https://codereview.appspot.com/6632044/

Search Discussions

  • Agl at Oct 9, 2012 at 6:19 pm
    https://codereview.appspot.com/6632044/diff/2002/src/pkg/crypto/hmac/hmac.go
    File src/pkg/crypto/hmac/hmac.go (right):

    https://codereview.appspot.com/6632044/diff/2002/src/pkg/crypto/hmac/hmac.go#newcode10
    src/pkg/crypto/hmac/hmac.go:10: Receivers should be careful to use
    Verify to compare MACs in order to avoid
    On 2012/10/09 15:43:28, rsc wrote:
    new paragraph here (insert blank line above this one)
    Done.

    https://codereview.appspot.com/6632044/diff/2002/src/pkg/crypto/hmac/hmac.go#newcode95
    src/pkg/crypto/hmac/hmac.go:95: // Verify compares two MACs for equality
    without leaking timing information.
    On 2012/10/09 15:43:28, rsc wrote:
    I'd prefer to use a name that is more clearly boolean. Also the
    variables could
    be named to make clearer that the arguments are the MACs.
    func Equal(mac1, mac2 []byte) bool
    ?
    Yes, that's clearly better. Thanks.
    func Sign(h func() hash.Hash, key, msg []byte) (mac []byte)
    func Verify(h func() hash.Hash, key, msg, mac []byte) error
    The problem is that it's very common (almost required) to hash in
    additional information to the HMAC. If the interface only supports
    passing in a single slice, people will either work around it or they'll
    have to alloc and copy a new slice.

    https://codereview.appspot.com/6632044/
  • Maxim Khitrov at Oct 9, 2012 at 7:20 pm

    On Tue, Oct 9, 2012 at 1:15 PM, wrote:
    func Sign(h func() hash.Hash, key, msg []byte) (mac []byte)
    func Verify(h func() hash.Hash, key, msg, mac []byte) error

    The problem is that it's very common (almost required) to hash in
    additional information to the HMAC. If the interface only supports
    passing in a single slice, people will either work around it or they'll
    have to alloc and copy a new slice.
    I'm currently working on a project that relies on the HMAC functions.
    I have a read-verify-read-verify-... loop, which is further
    complicated by the fact that certain HMAC comparisons are truncated to
    some number of most significant bytes. I think a single set of signing
    and verification functions is never going to fit all use cases, but a
    reminder that the comparison needs to be done in constant time is very
    useful.

    If you want a function that does more than a simple comparison of two
    byte slices, I would do something like this (what I use in my code):

    // Verify compares the HMAC tag in b with the current hash value
    // returned by h, truncated to t bytes. If len(b) > t, all preceding
    // bytes are written to h before the comparison. h.Size() is used
    // for t if t <= 0.
    func Verify(b []byte, t int, h hash.Hash)

    You can obviously remove t if you think that complicates the interface
    beyond what is acceptable in the standard library, but truncation is a
    normal part of HMAC operations (RFC 2104 section 5).
  • Adam Langley at Oct 9, 2012 at 9:23 pm

    On Tue, Oct 9, 2012 at 2:16 PM, Maxim Khitrov wrote:
    // Verify compares the HMAC tag in b with the current hash value
    // returned by h, truncated to t bytes. If len(b) > t, all preceding
    // bytes are written to h before the comparison. h.Size() is used
    // for t if t <= 0.
    func Verify(b []byte, t int, h hash.Hash)

    You can obviously remove t if you think that complicates the interface
    beyond what is acceptable in the standard library, but truncation is a
    normal part of HMAC operations (RFC 2104 section 5).
    I quite like that too, but I worry that it precludes avoiding the
    allocation each time when calling Sum. Without this function, users
    would are verifying many MACs can reuse storage. But, since
    escape-analysis isn't going to work through the interface, I think
    this would have to allocate every time.


    Cheers

    AGL

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedOct 9, '12 at 5:28p
activeOct 9, '12 at 9:23p
posts4
users3
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase