FAQ
Reviewers: agl1, dfc,

Message:
Hello agl@golang.org, dave@cheney.net (cc: golang-dev@googlegroups.com,
hanwen@google.com, jpsugar@google.com),

I'd like you to review this change to
https://code.google.com/p/go.crypto


Description:
go.crypto/ssh: Implement CertTime to fix an issue with some time
conversions.

Please review this at https://codereview.appspot.com/15520047/

Affected files (+36, -9 lines):
    M ssh/certs.go
    M ssh/keys_test.go


Index: ssh/certs.go
===================================================================
--- a/ssh/certs.go
+++ b/ssh/certs.go
@@ -35,6 +35,34 @@
    Data string
   }

+// A time.Time cannot represent seconds higher than 1<<63 - 1. However,
+// conversion from uint64 to int64 for values higher than this will result
in
+// negative int64 values and result in times before unix epoch that are not
+// intended to be used with certs. maxInt64 in unix seconds would be
+// 292277026596-12-04 15:30:07 +0000 UTC. We are safe until the year
+// 292,277,026,596 in setting this value to maxInt64 for values above
+// maxInt64. A use case for this is the "forever" setting where
ValidAfter is 0
+// (all bytes 0x00) and ValidBefore is 1<<64 - 1 (all bytes 0xFF).
OpenSSH does
+// something similar to this by clamping to INT_MAX.
+
+// CertTime represents an unsigned 64bit time value in seconds starting
from
+// unix epoch. We use CertTime instead of time.Time in order to properly
+// handle time values above what a time.Time can represent and prevent
values
+// before unix epoch.
+type CertTime uint64
+
+func (ct CertTime) Time() time.Time {
+ const maxInt64 = 1<<63 - 1
+ if ct > maxInt64 {
+ return time.Unix(maxInt64, 0)
+ }
+ return time.Unix(int64(ct), 0)
+}
+
+func (ct CertTime) IsTheEnd() bool {
+ return ct == 1<<64-1
+}
+
   // An OpenSSHCertV01 represents an OpenSSH certificate as defined in
   // [PROTOCOL.certkeys]?rev=1.8.
   type OpenSSHCertV01 struct {
@@ -44,7 +72,7 @@
    Type uint32
    KeyId string
    ValidPrincipals []string
- ValidAfter, ValidBefore time.Time
+ ValidAfter, ValidBefore CertTime
    CriticalOptions []tuple
    Extensions []tuple
    Reserved []byte
@@ -115,8 +143,8 @@
    r = marshalUint32(r, cert.Type)
    r = marshalString(r, []byte(cert.KeyId))
    r = marshalLengthPrefixedNameList(r, cert.ValidPrincipals)
- r = marshalUint64(r, uint64(cert.ValidAfter.Unix()))
- r = marshalUint64(r, uint64(cert.ValidBefore.Unix()))
+ r = marshalUint64(r, uint64(cert.ValidAfter))
+ r = marshalUint64(r, uint64(cert.ValidBefore))
    r = marshalTupleList(r, cert.CriticalOptions)
    r = marshalTupleList(r, cert.Extensions)
    r = marshalString(r, cert.Reserved)
@@ -195,13 +223,13 @@
    if !ok {
     return
    }
- cert.ValidAfter = time.Unix(int64(va), 0)
+ cert.ValidAfter = CertTime(va)

    vb, in, ok := parseUint64(in)
    if !ok {
     return
    }
- cert.ValidBefore = time.Unix(int64(vb), 0)
+ cert.ValidBefore = CertTime(vb)

    if cert.CriticalOptions, in, ok = parseTupleList(in); !ok {
     return
Index: ssh/keys_test.go
===================================================================
--- a/ssh/keys_test.go
+++ b/ssh/keys_test.go
@@ -9,7 +9,6 @@
    "reflect"
    "strings"
    "testing"
- "time"
   )

   var (
@@ -46,9 +45,9 @@
     Nonce: []byte{}, // To pass reflect.DeepEqual after marshal &
parse, this must be non-nil
     Key: ecdsaKey.PublicKey(),
     ValidPrincipals: []string{"gopher1", "gopher2"}, // increases test
coverage
- ValidAfter: time.Now().Truncate(time.Second),
- ValidBefore: time.Now().Truncate(time.Second).Add(time.Hour),
- Reserved: []byte{}, // To pass reflect.DeepEqual after marshal &
parse, this must be non-nil
+ ValidAfter: 0, // unix epoch
+ ValidBefore: 0xFFFFFFFFFFFFFFFF, // The end of currently
representable time.
+ Reserved: []byte{}, // To pass
reflect.DeepEqual after marshal & parse, this must be non-nil
     SignatureKey: rsaKey.PublicKey(),
    }
    sigBytes, _ := rsaKey.Sign(rand.Reader, testCert.BytesForSigning())


--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Search Discussions

  • Jpsugar at Oct 22, 2013 at 9:45 pm
    https://codereview.appspot.com/15520047/diff/40001/ssh/certs.go
    File ssh/certs.go (right):

    https://codereview.appspot.com/15520047/diff/40001/ssh/certs.go#newcode63
    ssh/certs.go:63: return ct == 1<<64-1
    Doesn't this result in an overflow?

    https://codereview.appspot.com/15520047/diff/40001/ssh/keys_test.go
    File ssh/keys_test.go (right):

    https://codereview.appspot.com/15520047/diff/40001/ssh/keys_test.go#newcode49
    ssh/keys_test.go:49: ValidBefore: 0xFFFFFFFFFFFFFFFF, //
    The end of currently representable time.
    Use a constant here?

    https://codereview.appspot.com/15520047/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Jmpittman at Oct 23, 2013 at 12:19 am
    https://codereview.appspot.com/15520047/diff/40001/ssh/certs.go
    File ssh/certs.go (right):

    https://codereview.appspot.com/15520047/diff/40001/ssh/certs.go#newcode63
    ssh/certs.go:63: return ct == 1<<64-1
    I do not think it does. I have not gotten an overflow in separate
    testing (i.e. playground). The math package defines this... MaxUint64
    = 1<<64 - 1, but I did not want to import the whole math package just
    for this.

    I can get 1<<64 to overflow, but I did not have 1<<64 - 1 overflow.

    https://codereview.appspot.com/15520047/diff/40001/ssh/keys_test.go
    File ssh/keys_test.go (right):

    https://codereview.appspot.com/15520047/diff/40001/ssh/keys_test.go#newcode49
    ssh/keys_test.go:49: ValidBefore: 0xFFFFFFFFFFFFFFFF, //
    The end of currently representable time.
    On 2013/10/22 21:45:35, jpsugar wrote:
    Use a constant here?
    Done.

    https://codereview.appspot.com/15520047/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Hanwen at Oct 23, 2013 at 5:03 am
    what problem is this CL solving?


    https://codereview.appspot.com/15520047/diff/40002/ssh/certs.go
    File ssh/certs.go (right):

    https://codereview.appspot.com/15520047/diff/40002/ssh/certs.go#newcode55
    ssh/certs.go:55: // handle time values above what a time.Time can
    represent and prevent values

    This comment is too large compared to what we're dealing with.

    time.Time goes until way beyond we all die because of petrol
    crisis/global warming/world war 3. It might even go beyond the time
    needed to brute-force RSA-2048, so it buys little additional security (I
    haven't checked though).

    https://codereview.appspot.com/15520047/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Jmpittman at Oct 23, 2013 at 11:34 am
    TLDR...
    1. Conversion from uint64 to int64 to time.Time and then back had issues
    with negative times (before unix epoch) which are not allowed in certs.
    2. Helps with parse/marshal when trying to work around issue 1.


    The field encoding is defined as a uint64, not an int64 like is becoming
    typical for representing time with 64bit values. It was done this way
    in the spec to avoid introducing a new wire encoding format for ssh
    (this was djm's reasoning when I asked). Also, valid times for
    certificates must start at unix epoch. So, a value of 0 is supposed to
    correspond to unix epoch.

    The problem this solves...

    In openssh, if a time frame/window is not specified for a certificate
    during creation, the default behavior is to set ValidAfter to all 0's
    and ValidBefore to all 1's (in binary form). This is known as the
    "forever" setting. Previously, the code was doing a direct conversion
    from uint64 to int64. Any value above 1<<63-1 (max for an int64) gets
    treated as a negative value. So, what should be considered a positive
    value (really far into the future) suddenly becomes the past (before
    unix epoch). Specifically, 0xFFFFFFFFFFFFFFFF in a uint64 is really far
    into the future, but that same value in an int64 is -1 or unix epoch - 1
    second. In the current form, when doing time validation on a cert, this
    will flip flop the window so that the certificate expires before it ever
    becomes valid.

    Introducing CertTime will clamp the time range to start at unix epoch
    and go to the max value for int64, but never before unix epoch. So, the
    time window is always positive.

    The other issue this solves is encoding. In trying to find a good way
    around this issue with just a time.Time value, everything I did would
    screw up the parse/marshal value comparison. If you want a test for
    this, we already have one. The example key that jpsugar provided has
    validafter at 0 and validbefore at all 0xFF bytes because he never set a
    time window. So, the cert is intended to always be good. With the
    current code, the certificate would have been expired one second before
    it became valid.

    Does that make sense?


    https://codereview.appspot.com/15520047/diff/40002/ssh/certs.go
    File ssh/certs.go (right):

    https://codereview.appspot.com/15520047/diff/40002/ssh/certs.go#newcode55
    ssh/certs.go:55: // handle time values above what a time.Time can
    represent and prevent values
    I wrote two comments. One for the reader of the code and one for
    package documentation. I was hoping the first one explained the
    reasoning for the CL.

    https://codereview.appspot.com/15520047/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Hanwen at Oct 23, 2013 at 3:32 pm
    https://codereview.appspot.com/15520047/diff/40002/ssh/certs.go
    File ssh/certs.go (right):

    https://codereview.appspot.com/15520047/diff/40002/ssh/certs.go#newcode55
    ssh/certs.go:55: // handle time values above what a time.Time can
    represent and prevent values
    ".. to properly handle the "infinite" time value ^0, which would become
    negative when expressed as int64"

    (I don't think anyone is concerned with any other value.)

    Likewise, the analysis of how this will be correct 300 billion years
    from now can be removed.

    https://codereview.appspot.com/15520047/diff/40002/ssh/certs.go#newcode66
    ssh/certs.go:66: func (ct CertTime) IsTheEnd() bool {
    IsInfinite?

    https://codereview.appspot.com/15520047/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Han-Wen Nienhuys at Oct 23, 2013 at 3:36 pm
    Also, can you change the description to be more explicit? Say how you
    fixed what issue.
    On Tue, Oct 22, 2013 at 2:36 PM, wrote:
    Reviewers: agl1, dfc,

    Message:
    Hello agl@golang.org, dave@cheney.net (cc: golang-dev@googlegroups.com,
    hanwen@google.com, jpsugar@google.com),

    I'd like you to review this change to
    https://code.google.com/p/go.crypto


    Description:
    go.crypto/ssh: Implement CertTime to fix an issue with some time
    conversions.

    Please review this at https://codereview.appspot.com/15520047/

    Affected files (+36, -9 lines):
    M ssh/certs.go
    M ssh/keys_test.go


    Index: ssh/certs.go
    ===================================================================
    --- a/ssh/certs.go
    +++ b/ssh/certs.go
    @@ -35,6 +35,34 @@
    Data string
    }

    +// A time.Time cannot represent seconds higher than 1<<63 - 1. However,
    +// conversion from uint64 to int64 for values higher than this will result
    in
    +// negative int64 values and result in times before unix epoch that are not
    +// intended to be used with certs. maxInt64 in unix seconds would be
    +// 292277026596-12-04 15:30:07 +0000 UTC. We are safe until the year
    +// 292,277,026,596 in setting this value to maxInt64 for values above
    +// maxInt64. A use case for this is the "forever" setting where ValidAfter
    is 0
    +// (all bytes 0x00) and ValidBefore is 1<<64 - 1 (all bytes 0xFF). OpenSSH
    does
    +// something similar to this by clamping to INT_MAX.
    +
    +// CertTime represents an unsigned 64bit time value in seconds starting
    from
    +// unix epoch. We use CertTime instead of time.Time in order to properly
    +// handle time values above what a time.Time can represent and prevent
    values
    +// before unix epoch.
    +type CertTime uint64
    +
    +func (ct CertTime) Time() time.Time {
    + const maxInt64 = 1<<63 - 1
    + if ct > maxInt64 {
    + return time.Unix(maxInt64, 0)
    + }
    + return time.Unix(int64(ct), 0)
    +}
    +
    +func (ct CertTime) IsTheEnd() bool {
    + return ct == 1<<64-1
    +}
    +
    // An OpenSSHCertV01 represents an OpenSSH certificate as defined in
    // [PROTOCOL.certkeys]?rev=1.8.
    type OpenSSHCertV01 struct {
    @@ -44,7 +72,7 @@
    Type uint32
    KeyId string
    ValidPrincipals []string
    - ValidAfter, ValidBefore time.Time
    + ValidAfter, ValidBefore CertTime
    CriticalOptions []tuple
    Extensions []tuple
    Reserved []byte
    @@ -115,8 +143,8 @@
    r = marshalUint32(r, cert.Type)
    r = marshalString(r, []byte(cert.KeyId))
    r = marshalLengthPrefixedNameList(r, cert.ValidPrincipals)
    - r = marshalUint64(r, uint64(cert.ValidAfter.Unix()))
    - r = marshalUint64(r, uint64(cert.ValidBefore.Unix()))
    + r = marshalUint64(r, uint64(cert.ValidAfter))
    + r = marshalUint64(r, uint64(cert.ValidBefore))
    r = marshalTupleList(r, cert.CriticalOptions)
    r = marshalTupleList(r, cert.Extensions)
    r = marshalString(r, cert.Reserved)
    @@ -195,13 +223,13 @@
    if !ok {
    return
    }
    - cert.ValidAfter = time.Unix(int64(va), 0)
    + cert.ValidAfter = CertTime(va)

    vb, in, ok := parseUint64(in)
    if !ok {
    return
    }
    - cert.ValidBefore = time.Unix(int64(vb), 0)
    + cert.ValidBefore = CertTime(vb)

    if cert.CriticalOptions, in, ok = parseTupleList(in); !ok {
    return
    Index: ssh/keys_test.go
    ===================================================================
    --- a/ssh/keys_test.go
    +++ b/ssh/keys_test.go
    @@ -9,7 +9,6 @@
    "reflect"
    "strings"
    "testing"
    - "time"
    )

    var (
    @@ -46,9 +45,9 @@
    Nonce: []byte{}, // To pass reflect.DeepEqual
    after marshal & parse, this must be non-nil
    Key: ecdsaKey.PublicKey(),
    ValidPrincipals: []string{"gopher1", "gopher2"}, //
    increases test coverage
    - ValidAfter: time.Now().Truncate(time.Second),
    - ValidBefore:
    time.Now().Truncate(time.Second).Add(time.Hour),
    - Reserved: []byte{}, // To pass reflect.DeepEqual
    after marshal & parse, this must be non-nil
    + ValidAfter: 0, // unix
    epoch
    + ValidBefore: 0xFFFFFFFFFFFFFFFF, // The end
    of currently representable time.
    + Reserved: []byte{}, // To pass
    reflect.DeepEqual after marshal & parse, this must be non-nil
    SignatureKey: rsaKey.PublicKey(),
    }
    sigBytes, _ := rsaKey.Sign(rand.Reader, testCert.BytesForSigning())


    --
    Han-Wen Nienhuys
    Google Munich
    hanwen@google.com

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Jmpittman at Oct 23, 2013 at 4:30 pm
    https://codereview.appspot.com/15520047/diff/40002/ssh/certs.go
    File ssh/certs.go (right):

    https://codereview.appspot.com/15520047/diff/40002/ssh/certs.go#newcode55
    ssh/certs.go:55: // handle time values above what a time.Time can
    represent and prevent values
    On 2013/10/23 15:32:50, hanwen-google wrote:
    ".. to properly handle the "infinite" time value ^0, which would
    become negative
    when expressed as int64"
    (I don't think anyone is concerned with any other value.)
    Likewise, the analysis of how this will be correct 300 billion years from now
    can be removed.
    Done.

    https://codereview.appspot.com/15520047/diff/40002/ssh/certs.go#newcode66
    ssh/certs.go:66: func (ct CertTime) IsTheEnd() bool {
    On 2013/10/23 15:32:50, hanwen-google wrote:
    IsInfinite?
    Done.

    https://codereview.appspot.com/15520047/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Agl at Oct 23, 2013 at 4:40 pm
    https://codereview.appspot.com/15520047/diff/160001/ssh/certs.go
    File ssh/certs.go (right):

    https://codereview.appspot.com/15520047/diff/160001/ssh/certs.go#newcode39
    ssh/certs.go:39: maxUint64 = 1<<64 - 1
    for future reference, there's math.MaxUint64 and math.MaxInt64. But, in
    this case, importing math for that seems too much.

    https://codereview.appspot.com/15520047/diff/160001/ssh/certs.go#newcode43
    ssh/certs.go:43: // CertTime represents an unsigned 64bit time value in
    seconds starting from
    "64-bit"

    https://codereview.appspot.com/15520047/diff/160001/ssh/certs.go#newcode44
    ssh/certs.go:44: // unix epoch. We use CertTime instead of time.Time in
    order to properly handle
    s/unix/UNIX/

    https://codereview.appspot.com/15520047/diff/160001/ssh/certs.go#newcode46
    ssh/certs.go:46: // int64.
    "an int64"

    https://codereview.appspot.com/15520047/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Jmpittman at Oct 23, 2013 at 4:41 pm
    https://codereview.appspot.com/15520047/diff/160001/ssh/certs.go
    File ssh/certs.go (right):

    https://codereview.appspot.com/15520047/diff/160001/ssh/certs.go#newcode39
    ssh/certs.go:39: maxUint64 = 1<<64 - 1
    That was my reasoning for doing this.

    https://codereview.appspot.com/15520047/diff/160001/ssh/certs.go#newcode43
    ssh/certs.go:43: // CertTime represents an unsigned 64bit time value in
    seconds starting from
    On 2013/10/23 16:38:37, agl1 wrote:
    "64-bit"
    Done.

    https://codereview.appspot.com/15520047/diff/160001/ssh/certs.go#newcode43
    ssh/certs.go:43: // CertTime represents an unsigned 64bit time value in
    seconds starting from
    On 2013/10/23 16:38:37, agl1 wrote:
    "64-bit"
    Done.

    https://codereview.appspot.com/15520047/diff/160001/ssh/certs.go#newcode44
    ssh/certs.go:44: // unix epoch. We use CertTime instead of time.Time in
    order to properly handle
    On 2013/10/23 16:38:37, agl1 wrote:
    s/unix/UNIX/
    Done.

    https://codereview.appspot.com/15520047/diff/160001/ssh/certs.go#newcode46
    ssh/certs.go:46: // int64.
    On 2013/10/23 16:38:37, agl1 wrote:
    "an int64"
    Done.

    https://codereview.appspot.com/15520047/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Agl at Oct 23, 2013 at 4:44 pm
    *** Submitted as
    https://code.google.com/p/go/source/detail?r=a7997f1dd284&repo=crypto
    ***

    go.crypto/ssh: Implement CertTime to properly handle the "infinite" time
    value ^0, which would become negative when expressed as int64.

    R=agl, dave, jpsugar, hanwen
    CC=golang-dev
    https://codereview.appspot.com/15520047

    Committer: Adam Langley <agl@golang.org>


    https://codereview.appspot.com/15520047/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedOct 22, '13 at 9:36p
activeOct 23, '13 at 4:44p
posts11
users4
websitegolang.org

People

Translate

site design / logo © 2021 Grokbase