Repository: google/certificate-transparency-go
Pull request #626 (2019-12-02 15:05:28)

Factor out conversion from AddChainResponse to SCT.
This is mostly extracted from what is done in
LogClient.addChainWithRetry, but with a stronger check on the length of
the id field in the AddChainResponse.  Unfortunatly this additional check makes
it non-trivial to refactof LogClient.addChainWithRetry, as the SCT used
in the tests for that function use an SCT with an empty ID field.  Thus,
that refactoring will come in a follow up PR.

### Checklist

- [ ] I have updated the [CHANGELOG](CHANGELOG.md).
  - Adjust the draft version number according to [semantic versioning](https://semver.org/) rules.
- [ ] I have updated [documentation](docs/) accordingly.


codecov[bot]: # [Codecov](https://codecov.io/gh/google/certificate-transparency-go/pull/626?src=pr&el=h1) Report
> Merging [#626](https://codecov.io/gh/google/certificate-transparency-go/pull/626?src=pr&el=desc) into [master](https://codecov.io/gh/google/certificate-transparency-go/commit/90efc49420797623b32cf3aa46071c8da4439fe6?src=pr&el=desc) will **increase** coverage by `0.02%`.
> The diff coverage is `83.33%`.

[![Impacted file tree graph](https://codecov.io/gh/google/certificate-transparency-go/pull/626/graphs/tree.svg?width=650&token=kYPnjCLppA&height=150&src=pr)](https://codecov.io/gh/google/certificate-transparency-go/pull/626?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #626      +/-   ##
==========================================
+ Coverage   71.73%   71.75%   +0.02%     
==========================================
  Files          90       90              
  Lines        9835     9853      +18     
==========================================
+ Hits         7055     7070      +15     
- Misses       2285     2286       +1     
- Partials      495      497       +2
```


| [Impacted Files](https://codecov.io/gh/google/certificate-transparency-go/pull/626?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [types.go](https://codecov.io/gh/google/certificate-transparency-go/pull/626/diff?src=pr&el=tree#diff-dHlwZXMuZ28=) | `39.06% <83.33%> (+7.24%)` | :arrow_up: |
| [fixchain/logger.go](https://codecov.io/gh/google/certificate-transparency-go/pull/626/diff?src=pr&el=tree#diff-Zml4Y2hhaW4vbG9nZ2VyLmdv) | `80.68% <0%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/google/certificate-transparency-go/pull/626?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/google/certificate-transparency-go/pull/626?src=pr&el=footer). Last update [90efc49...2e2db83](https://codecov.io/gh/google/certificate-transparency-go/pull/626?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/types.go b/types.go
index 64efc9d2..40ed60ab 100644
--- a/types.go
+++ b/types.go
@@ -462,6 +462,36 @@ type AddChainResponse struct {
 	Signature  []byte  `json:"signature"`   // Log signature for this SCT
 }
 
+// ToSignedCertificateTimestamp creates a SignedCertificateTimestamp from the
+// AddChainResponse.
+func (r *AddChainResponse) ToSignedCertificateTimestamp() (*SignedCertificateTimestamp, error) {
+	sct := SignedCertificateTimestamp{
+		SCTVersion: r.SCTVersion,
+		Timestamp:  r.Timestamp,
+	}
+
+	if len(r.ID) != sha256.Size {
+		return nil, fmt.Errorf("id is invalid length, expected %d got %d", sha256.Size, len(r.ID))
+	}
+	copy(sct.LogID.KeyID[:], r.ID)
+
+	exts, err := base64.StdEncoding.DecodeString(r.Extensions)
+	if err != nil {
+		return nil, fmt.Errorf("invalid base64 data in Extensions (%q): %v", r.Extensions, err)
+	}
+	sct.Extensions = CTExtensions(exts)
+
+	var ds DigitallySigned
+	if rest, err := tls.Unmarshal(r.Signature, &ds); err != nil {
+		return nil, fmt.Errorf("tls.Unmarshal(): %s", err)
+	} else if len(rest) > 0 {
+		return nil, fmt.Errorf("trailing data (%d bytes) after DigitallySigned", len(rest))
+	}
+	sct.Signature = ds
+
+	return &sct, nil
+}
+
 // AddJSONRequest represents the JSON request body sent to the add-json POST method.
 // The corresponding response re-uses AddChainResponse.
 // This is an experimental addition not covered by RFC6962.


Test patch:
diff --git a/types_test.go b/types_test.go
index 271c4aa5..75e0a648 100644
--- a/types_test.go
+++ b/types_test.go
@@ -15,7 +15,9 @@
 package ct
 
 import (
+	"encoding/base64"
 	"encoding/hex"
+	"fmt"
 	"strings"
 	"testing"
 
@@ -65,6 +67,80 @@ func TestUnmarshalMerkleTreeLeaf(t *testing.T) {
 	}
 }
 
+func mustB64Decode(s string) []byte {
+	b, err := base64.StdEncoding.DecodeString(s)
+	if err != nil {
+		fmt.Println(s)
+		panic(err)
+	}
+	return b
+}
+
+func TestToSignedCertificateTimestamp(t *testing.T) {
+	// From the sct:
+	// {"sct_version":0,"id":"CEEUmABxUywWGQRgvPxH/cJlOvopLHKzf/hjrinMyfA=","timestamp":1512556025588,"extensions":"","signature":"BAMARjBEAiAJAPO7EKykH4eOQ81kTzKCb4IEWzcxTBdbdRCHLFPLFAIgBEoGXDUtcIaF3M5HWI+MxwkCQbvqR9TSGUHDCZoOr3Q="}
+	validLogID := "CEEUmABxUywWGQRgvPxH/cJlOvopLHKzf/hjrinMyfA="
+	longLogID := "CEEUmABxUywWGQRgvPxH/cJlOvopLHKzf/hjrinMyfA0"
+	shortLogID := "CEEUmABxUywWGQRgvPxH/cJlOvopLHKzf/hjrinMyf=="
+	validSCTSignature := "BAMARjBEAiAJAPO7EKykH4eOQ81kTzKCb4IEWzcxTBdbdRCHLFPLFAIgBEoGXDUtcIaF3M5HWI+MxwkCQbvqR9TSGUHDCZoOr3Q="
+	longSCTSignature := "BAMARjBEAiAJAPO7EKykH4eOQ81kTzKCb4IEWzcxTBdbdRCHLFPLFAIgBEoGXDUtcIaF3M5HWI+MxwkCQbvqR9TSGUHDCZoOr3Q0"
+
+	tests := []struct {
+		desc      string
+		logID     string
+		exts      string
+		signature string
+		wantErr   bool
+	}{
+		{
+			desc:      "success",
+			logID:     validLogID,
+			signature: validSCTSignature,
+		},
+		{
+			desc:      "log ID too long",
+			logID:     longLogID,
+			signature: validSCTSignature,
+			wantErr:   true,
+		},
+		{
+			desc:      "log ID too short",
+			logID:     shortLogID,
+			signature: validSCTSignature,
+			wantErr:   true,
+		},
+		{
+			desc:      "extensions not base64",
+			logID:     validLogID,
+			exts:      "This is not Base64",
+			signature: validSCTSignature,
+			wantErr:   true,
+		},
+		{
+			desc:      "signature trailing data",
+			logID:     validLogID,
+			signature: longSCTSignature,
+			wantErr:   true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.desc, func(t *testing.T) {
+			sctResponse := &AddChainResponse{
+				SCTVersion: 0,
+				ID:         mustB64Decode(test.logID),
+				Timestamp:  1512556025588,
+				Extensions: test.exts,
+				Signature:  mustB64Decode(test.signature),
+			}
+			sct, err := sctResponse.ToSignedCertificateTimestamp()
+			if gotErr := err != nil; gotErr != test.wantErr {
+				t.Errorf("AddChainResponse.ToSignedCertificateTimestamp() = %+v, %v, want err? %t", sct, err, test.wantErr)
+			}
+		})
+	}
+}
+
 const (
 	validRootHash = "708981e91d1487c2a9ea901ab5a8d053c1348585afcdb5e107bf60c0c1d20fc0"
 	longRootHash  = "708981e91d1487c2a9ea901ab5a8d053c1348585afcdb5e107bf60c0c1d20fc000"