trust: create TLS configuration based on CPPKI

[JordiSubira]

Add a crypto manager that provides the necessary callbacks to create a SCION CPPKI backed tls.Conifg.

---

This change is 

[oncilla]

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @JordiSubira)

---

go/pkg/cs/trust/tls_handshake.go, line 1 at r1 (raw file):

// Copyright 2020 ETH Zurich

Should be formatted like so:

// Copyright 2020 ETH Zurich
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//   http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

---

go/pkg/cs/trust/tls_handshake.go, line 28 at r1 (raw file):

)

// X509KeyPairLoader provides a certificate to be presented during TLS HS.

Please expand HS to handshake here and below.

---

go/pkg/cs/trust/tls_handshake.go, line 43 at r1 (raw file):

	c, err := m.Loader.LoadX509KeyPair()
	if err != nil {
		return nil, serrors.WrapStr("Error loading server key pair", err)

error strings should not be capitalized: https://github.com/golang/go/wiki/CodeReviewComments#error-strings

for better compsability, you can drop the error here.

serrors.WrapStr("loading server key pair", err)

---

go/pkg/cs/trust/tls_handshake.go, line 53 at r1 (raw file):

	c, err := m.Loader.LoadX509KeyPair()
	if err != nil {
		return nil, serrors.WrapStr("Error loading client key pair", err)

ditto

---

go/pkg/cs/trust/tls_handshake.go, line 60 at r1 (raw file):

// VerifyPeerCertificate verifies the certificate presented by the peer during TLS HS,
// based on the TRC.
func (m *TLSCryptoManager) VerifyPeerCertificate(rawCerts [][]byte,

to make explicit that we never use the verified chains, replace it with an underscore:

func (m *TLSCryptoManager) VerifyPeerCertificate(rawCerts [][]byte, _ [][]*x509.Certificate) error {

---

go/pkg/cs/trust/tls_handshake.go, line 67 at r1 (raw file):

		cert, err := x509.ParseCertificate(asn1Data)
		if err != nil {
			return serrors.New("failed to parse peer certificate", "err", err)

return serrors.WrapStr("parsing peer certificate", err)

---

go/pkg/cs/trust/tls_handshake.go, line 71 at r1 (raw file):

		chain[i] = cert
	}
	ia, err := cppki.ExtractIA(chain[0].Subject)

The error can be nil, and IA can still be nil. This would cause a panic if a non-AS certificate is provided during the handshake.

ia, err := cppki.ExtractIA(chain[0].Subject)
if err != nil {
    return serrors.WrapStr("extracting ISD-AS from peer certificate", err)
}else if ia == nil {
    return serrors.WrapStr("ISD-AS no present in peer certificate")
}

---

go/pkg/cs/trust/tls_handshake.go, line 75 at r1 (raw file):

		return serrors.WrapStr("error extracting IA from peer cert", err)
	}
	trc, err := m.DB.SignedTRC(context.Background(), cppki.TRCID{

This only fetches the latest TRC, if we are in grace period, we have two potential TRCs that could be used to verify.

ctx, cancel := context.WithTimeout(context.Background(), m.Timeout)
defer cancel()
trcs, _, err := activeTRCs(context.Background(), m.DB, ia.I)
if err != nil {
  ...
}
if err := verifyChain(chain, trcs); err != nil {
   ...
}
return nil

In fetching_provider.go you can extract the inner loop from filterVerifiableChains

func verifyChain(chain []*x509.Certificate, trcs []cppki.SignedTRC) error {
    var errs serrros.List
    for _, trc := range trcs {
        if err := cppki.VerifyChain(chain, cppki.VerifyOptions{TRC: &trc.TRC}); err != nil {
            errs = append(errs, err)
        }
    }
    return errs.ToError()
}

---

go/pkg/cs/trust/tls_handshake_test.go, line 1 at r1 (raw file):

// Copyright 2020 ETH Zurich

ditto regarding formatting

---

go/pkg/cs/trust/tls_handshake_test.go, line 31 at r1 (raw file):

	"github.com/scionproto/scion/go/pkg/trust/mock_trust"
)

It would be really cool if there was a test that shows dialing a TLS connection is possible using the TLSCryptoManager.
not really needed for this to be merged though.

---

go/pkg/cs/trust/tls_handshake_test.go, line 32 at r1 (raw file):

)

func TestVerifyPeerCertificate(t *testing.T) {

should be TestTLSCryptoManagerVerifyPeerCertificate

---

go/pkg/cs/trust/tls_handshake_test.go, line 37 at r1 (raw file):

	testCases := map[string]struct {
		prepare   func(t *testing.T, ctrl *gomock.Controller) (string, trust.DB)

I would prefer if these are separated by concerns.

map[string]struct {
    db func(*gomock.Controller) trust.DB 
    assertion assert.ErrorAssertionFunc
    // I don't think you need crt111File as a return value at this point.
    // We can add it trivially, when we will need it later on.
}

---

go/pkg/cs/trust/tls_handshake_test.go, line 66 at r1 (raw file):

}

func loadRawChain(t *testing.T, file string) [][]byte {

this can be simplified to:

var raw [][]byte
for _, cert := range xtest.LoadChain(t, file) {
    raw = append(raw, cert.Raw)
}
return raw

[JordiSubira]

Reviewable status: 0 of 4 files reviewed, 13 unresolved discussions (waiting on @oncilla)

---

go/pkg/cs/trust/tls_handshake.go, line 1 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

Should be formatted like so:

// Copyright 2020 ETH Zurich
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//   http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

Done.

---

go/pkg/cs/trust/tls_handshake.go, line 28 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

Please expand HS to handshake here and below.

Done.

---

go/pkg/cs/trust/tls_handshake.go, line 43 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

error strings should not be capitalized: https://github.com/golang/go/wiki/CodeReviewComments#error-strings

for better compsability, you can drop the error here.

serrors.WrapStr("loading server key pair", err)

Done.

---

go/pkg/cs/trust/tls_handshake.go, line 53 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

ditto

Done.

---

go/pkg/cs/trust/tls_handshake.go, line 60 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

to make explicit that we never use the verified chains, replace it with an underscore:

func (m *TLSCryptoManager) VerifyPeerCertificate(rawCerts [][]byte, _ [][]*x509.Certificate) error {

Done.

---

go/pkg/cs/trust/tls_handshake.go, line 67 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

return serrors.WrapStr("parsing peer certificate", err)

Done.

---

go/pkg/cs/trust/tls_handshake.go, line 71 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

The error can be nil, and IA can still be nil. This would cause a panic if a non-AS certificate is provided during the handshake.

ia, err := cppki.ExtractIA(chain[0].Subject)
if err != nil {
    return serrors.WrapStr("extracting ISD-AS from peer certificate", err)
}else if ia == nil {
    return serrors.WrapStr("ISD-AS no present in peer certificate")
}

Done.

---

go/pkg/cs/trust/tls_handshake.go, line 75 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

This only fetches the latest TRC, if we are in grace period, we have two potential TRCs that could be used to verify.

ctx, cancel := context.WithTimeout(context.Background(), m.Timeout)
defer cancel()
trcs, _, err := activeTRCs(context.Background(), m.DB, ia.I)
if err != nil {
  ...
}
if err := verifyChain(chain, trcs); err != nil {
   ...
}
return nil

In fetching_provider.go you can extract the inner loop from filterVerifiableChains

func verifyChain(chain []*x509.Certificate, trcs []cppki.SignedTRC) error {
    var errs serrros.List
    for _, trc := range trcs {
        if err := cppki.VerifyChain(chain, cppki.VerifyOptions{TRC: &trc.TRC}); err != nil {
            errs = append(errs, err)
        }
    }
    return errs.ToError()
}

Done.

---

go/pkg/cs/trust/tls_handshake_test.go, line 1 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

ditto regarding formatting

Done.

---

go/pkg/cs/trust/tls_handshake_test.go, line 31 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

It would be really cool if there was a test that shows dialing a TLS connection is possible using the TLSCryptoManager.
not really needed for this to be merged though.

I added one test doing a handshake between parties. Please take a look and provide any suggestions to improve it, if necessary.

---

go/pkg/cs/trust/tls_handshake_test.go, line 32 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

should be TestTLSCryptoManagerVerifyPeerCertificate

Done.

---

go/pkg/cs/trust/tls_handshake_test.go, line 37 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

I would prefer if these are separated by concerns.

map[string]struct {
    db func(*gomock.Controller) trust.DB 
    assertion assert.ErrorAssertionFunc
    // I don't think you need crt111File as a return value at this point.
    // We can add it trivially, when we will need it later on.
}

Done.

---

go/pkg/cs/trust/tls_handshake_test.go, line 66 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

this can be simplified to:

var raw [][]byte
for _, cert := range xtest.LoadChain(t, file) {
    raw = append(raw, cert.Raw)
}
return raw

Done.

[oncilla]

Reviewed 4 of 7 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JordiSubira)

---

go/pkg/trust/tls_handshake.go, line 102 at r2 (raw file):

	var errs serrors.List
	for _, trc := range trcs {
		if err := cppki.VerifyChain(chain, cppki.VerifyOptions{TRC: &trc.TRC}); err != nil {

My bad, you need to return nil error if any of the verifications is successful:

func verifyChain(chain []*x509.Certificate, trcs []cppki.SignedTRC) error {
    var errs serrros.List
    for _, trc := range trcs {
        if err := cppki.VerifyChain(chain, cppki.VerifyOptions{TRC: &trc.TRC}); err != nil {
            errs = append(errs, err)
            continue
        }
        return nil
    }
    return errs.ToError()
}

[JordiSubira]

Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @oncilla)

---

go/pkg/trust/tls_handshake.go, line 102 at r2 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

My bad, you need to return nil error if any of the verifications is successful:

func verifyChain(chain []*x509.Certificate, trcs []cppki.SignedTRC) error {
    var errs serrros.List
    for _, trc := range trcs {
        if err := cppki.VerifyChain(chain, cppki.VerifyOptions{TRC: &trc.TRC}); err != nil {
            errs = append(errs, err)
            continue
        }
        return nil
    }
    return errs.ToError()
}

Done.

[oncilla]

Reviewed 1 of 1 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

[oncilla]

Reviewable status: complete! all files reviewed, all discussions resolved