Code review - Issue 15520047: code review 15520047: go.crypto/ssh: Implement CertTime to fix an issue with ...https://codereview.appspot.com/2013-10-23T16:44:36+00:00rietveld
Message from unknown
2013-10-22T21:19:02+00:00jmpittmanurn:md5:d88e1ea7a9cf7ef1033b4a6027c3193c
Message from unknown
2013-10-22T21:19:08+00:00jmpittmanurn:md5:ca9b355017fb0be1549507625252598b
Message from unknown
2013-10-22T21:36:07+00:00jmpittmanurn:md5:1798b04a3404965f55463e91e96b85a6
Message from jmpittman@google.com
2013-10-22T21:36:11+00:00jmpittmanurn:md5:26918e5d2541c9200c9e030442481289
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
Message from jpsugar@google.com
2013-10-22T21:45:35+00:00jpsugarurn:md5:a93d6d36f811c79e93b5d49f75d32940
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?
Message from unknown
2013-10-23T00:18:34+00:00jmpittmanurn:md5:79300792b8b65cd51f0855b02d4a43ed
Message from jmpittman@google.com
2013-10-23T00:19:01+00:00jmpittmanurn:md5:f8790515ab9419082a21e84d7c4ba79f
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.
Message from hanwen@google.com
2013-10-23T05:03:28+00:00hanwen-googleurn:md5:0c01b4413f6cc5eb1b3b24d3876f3869
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).
Message from jmpittman@google.com
2013-10-23T11:34:48+00:00jmpittmanurn:md5:391d6f18701ed29baa91772b4caf969d
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.
Message from hanwen@google.com
2013-10-23T15:32:49+00:00hanwen-googleurn:md5:db1093e1f4fb4849c54d4e04e41fe3e1
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?
Message from hanwen@google.com
2013-10-23T15:36:44+00:00hanwen-googleurn:md5:83a544090e10b6b983d15b662b5ac6d1
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
Message from unknown
2013-10-23T16:27:03+00:00jmpittmanurn:md5:3422a694924a1d57bae35304268018cb
Message from unknown
2013-10-23T16:29:46+00:00jmpittmanurn:md5:1cc070e954bb97bd8a88ba9cc004dba2
Message from jmpittman@google.com
2013-10-23T16:30:33+00:00jmpittmanurn:md5:85a49c9cd9a4fe64460ff5166a80b63d
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.
Message from agl@golang.org
2013-10-23T16:38:36+00:00agl1urn:md5:4400071d07a2de653d5e0e0f25f50a30
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"
Message from unknown
2013-10-23T16:40:38+00:00jmpittmanurn:md5:789f28ca49a7198220337e36626fa9ee
Message from jmpittman@google.com
2013-10-23T16:41:05+00:00jmpittmanurn:md5:fe995b630ad6caaeed5db0859e8a4bad
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.
Message from agl@golang.org
2013-10-23T16:44:36+00:00agl1urn:md5:bb00ba8bc8bcd9ab4b973afe34c792d8
*** 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>