Issue 15520047: code review 15520047: go.crypto/ssh: Implement CertTime to fix an issue with ...

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 ...

10 years, 6 months ago (2013-10-22 21:36:11 UTC) #1

jpsugar

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 ...

10 years, 6 months ago (2013-10-22 21:45:35 UTC) #2

jmpittman

10 years, 6 months ago (2013-10-23 00:19:01 UTC) #3

hanwen-google

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 ...

10 years, 6 months ago (2013-10-23 05:03:28 UTC) #4

jmpittman

TLDR... 1. Conversion from uint64 to int64 to time.Time and then back had issues with ...

10 years, 6 months ago (2013-10-23 11:34:48 UTC) #5

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.

hanwen-google

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 ...

10 years, 6 months ago (2013-10-23 15:32:49 UTC) #6

hanwen-google

Also, can you change the description to be more explicit? Say how you fixed what ...

10 years, 6 months ago (2013-10-23 15:36:44 UTC) #7

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,  <jmpittman@google.com> 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

jmpittman

10 years, 6 months ago (2013-10-23 16:30:33 UTC) #8

agl1

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 ...

10 years, 6 months ago (2013-10-23 16:38:36 UTC) #9

jmpittman

10 years, 6 months ago (2013-10-23 16:41:05 UTC) #10

agl1

10 years, 6 months ago (2013-10-23 16:44:36 UTC) #11

*** 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>

Expand All Messages | Collapse All Messages