From d66c7ab85786ab3c0ce22caed90ccd34a024dff0 Mon Sep 17 00:00:00 2001
From: Patrick Zheng <patrickzheng@microsoft.com>
Date: Sat, 2 Nov 2024 01:40:07 +0800
Subject: [PATCH] fix: added tsa trust store root cert validation (#471)

This PR adds tsa trust store root cert validation while getting
certificates from trust store. This is to fail fast if cert in TSA trust
store is not a root CA certificate.

Resolves #470

---------

Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
---
 .../test-mismatch/DigiCertTSARootSHA384.cer   | Bin 0 -> 1428 bytes
 .../wabbit-networks.io.crt                    |   0
 .../tsa/test-nonSelfIssued/nonSelfIssued.crt  | Bin 0 -> 1309 bytes
 verifier/truststore/truststore.go             |  20 +++++++++++++
 verifier/truststore/truststore_test.go        |  28 ++++++++++++++++++
 5 files changed, 48 insertions(+)
 create mode 100644 verifier/testdata/truststore/x509/tsa/test-mismatch/DigiCertTSARootSHA384.cer
 rename verifier/testdata/truststore/x509/tsa/{test-mismatch => test-nonCA}/wabbit-networks.io.crt (100%)
 create mode 100644 verifier/testdata/truststore/x509/tsa/test-nonSelfIssued/nonSelfIssued.crt

diff --git a/verifier/testdata/truststore/x509/tsa/test-mismatch/DigiCertTSARootSHA384.cer b/verifier/testdata/truststore/x509/tsa/test-mismatch/DigiCertTSARootSHA384.cer
new file mode 100644
index 0000000000000000000000000000000000000000..99bcc84b7e68b5b28e4444f6fa21bc7c2baf497d
GIT binary patch
literal 1428
zcmXqLVx3^n#9Xm}nTe5!Nq}{>bojhJMWaWS?0c7&m&O?IvT<s)d9;1!Wn|=GWiUuG
z<Tl`BV-96u6J`nxHWW1w264EAd0aBnGo4e5N)$Zvk_{yd#6iN`!UE;x<$5VVsbrv(
zUUGh}p`w92NQzll0<I&ZsI<5wHANvPKfgr5-NZmnoY&CUz{0@L(8vG?qQrTPK^#*k
zmo9E-Vp2l(IwLCsa}yIkgFzD$7gG}xBg6h@6N)qL?S2yLWn-XZkZJtp&~jD1lZv*V
zx^6@sdZ@YFHez+zth4K?rCWH97|4H?USc8kI_%i=T-nu|DnFePy!0gg8mmnZe~|pK
zd7?|}_RU`zE*2SDv_4~U!J$pNN@r#j8%?<=;3A-Rzv(l3%d-;e+`>n?JkswJuAWqo
zP1e*s_a@Ho#N;1}iL*^!vmT3k6D_sp^~v*R*O)lOZ>&mtSAN1{MOt|H{E&z~9_{V^
z%MEUZy*pJM`*`h1|G1~7&kaxCnjCkhufO5ewuv(wCR84-IKFM;k*!%07R&;@H?Ej3
z(PORc_}XMAFtK2DXp^JS_1i4PT6q&0YZQI1>{%zxTpC-EcGJqxWtOqSeva!=o=Xlr
zTe%?p?h^Gq3;iv(3Py;3SBY`!Px*c@v!iTAnQdgOQ(1fG^vo)c4-XazNvF*!Id#ul
z?m1ubx@TA3Pnu*k&-M<(6Ia#FZL?e?wd)Q{*>Wi{_qFlOqxZd87|ztnOg-HHU2)SU
z!R@>2KV9u9&~Z#ywJ}-3WvWzJQr)+P4ZmNcEHl2?$^LNf_GivZBz7z-XMD&%g-20#
zQ;4Q&XU<c;pg{MMeQImAIhNk5S`*)Ee{#=dM?s<Jrvd@54Ue8G5>w*5U*l71pZrEK
z{)j?gcK*iIZQcHduDQm~Rrs?|?&yL3MH}n5)MkEtlBqvKR`=`8m78RrN;5GtGB7T7
zGH@{92PS7(VMfOPEUX61K+1p*B)|_6U;*Z-HUn7@pN~b1MdZ!($4!?CV^e(Y>!sU2
z-!)^M48K2eDg$OPU@Bu|*qwN@c4f{!@gozZ4=-HA(EB(ggFozi`MQFie`k5k+<CGh
zhKDEq$ukknFm5x?SeN7nYv)XpNRw9PwXJ-an|&?h$0eJC&JOEj1HK0RnP9zp#>~v@
z;|G_o$XKF&XYNn+bq1|Fzoq+H+4V<SDh0c|J`u}&>TN((ZIU8a!-?Xwiwjs2;$JM?
zvV8rD@42RPYNEQXEwY&TxuW}v?<EE9do1sIX1g~Q@g5K{dT$V^x9&mVr?4kk)AJ_<
z<WB3&sG7COOr(nKlgds`!GP-=8+UY8wJ0Y#X+B+ic9DYP^^0>-ZoX1m*e!D1M{-ku
zW1+3RZ-H_fkJp{XOJ|IxwD59pPM7gN@Ge`S#Ng5cOA~=sMNvkM7okS?3O#RXhzIyS
z+vj_+bj^iRza4itFI{!{FsqBdj@j%-zaF{nP!7&v%TEujciZY?pQjO3sdj0}ilph6
z<Q~2C>fR&$*WHWvetKHnrfA0t)=$1ze_=^}`TkG{L*Rlgt&`^}&Rl)f&LXk-+<MQB
z^`9rr+_w2yBLAgUhUWoNPo$^Lm0Y-U&c@jr8RXty;P-dRImxx>o%1iN!lfsC)s~w6
z?Tp>inj~v<X@Pdg+gafUZ~qcFFS)(jOS+$NoBGt?7uz~>qxWQ7QGU@KxG3!Bhh+^5
ouE}lQ!_OaFs=4ZwaQTyaJ&lTM*#+DM*S6cTUo72o{&QL#0GZBIfB*mh

literal 0
HcmV?d00001

diff --git a/verifier/testdata/truststore/x509/tsa/test-mismatch/wabbit-networks.io.crt b/verifier/testdata/truststore/x509/tsa/test-nonCA/wabbit-networks.io.crt
similarity index 100%
rename from verifier/testdata/truststore/x509/tsa/test-mismatch/wabbit-networks.io.crt
rename to verifier/testdata/truststore/x509/tsa/test-nonCA/wabbit-networks.io.crt
diff --git a/verifier/testdata/truststore/x509/tsa/test-nonSelfIssued/nonSelfIssued.crt b/verifier/testdata/truststore/x509/tsa/test-nonSelfIssued/nonSelfIssued.crt
new file mode 100644
index 0000000000000000000000000000000000000000..6ec500520a70677de8fe99e14e217bc41f44be7c
GIT binary patch
literal 1309
zcmXqLVwE&#Vs2c(%*4pV#Kf@BfR~L^tIebBJ1-+6H!Fidu_3nsCmVAp3!5-gXfTY!
zA<Pu+XeeO72a;hIW)DtHEGfxJHRK0s2a0eBv-#zhBo<W~niv>?1ek@jK>{V2`FRQ<
zsl_D<L8)c=*@;OxsR}{Cjtb5hiJ5r{&Z$K}iTwN$137VCBNIadBTEAd19L;uC<xcs
z0LnEefVqP_M;RLELmZ_+q@#=sniv-$hYTYt19KB&KLb#_i>ZmRk>LXCs%sMu?+!8w
z3(n9`Ru})z<dSiE%FjdZcYmGMZl7!r)gjc(IAgK!D+3di@|17WS{#-=x-@a}o&_Rn
zr_ZWzU;3?{<FD`zyTp^aD<8SJBt&PJv9D5!zaTjIfssY&h9f5*ImJ(!*Zi~UWP9e$
zgb$r=T8~$hX5_6sZC6pc?z-^dQ>-RGfBJbGt&RE}cm89ZKyUxgOqJ|X`#BG8w%=X5
z<saY6N1F_zV|Tri|Du!7awu?~xx~bWaTio><Q>*Lr6svGv1%EMFrV3ud9h0Zc5{3C
zn3+VjWQF`cXv_R-$6TQczGlxaeR_A#J-hzWe{*r>c;%dMKYrgyN_=_II>PBYU$1A{
z-6kiOeZaBb;Dk2&-u<e}j_%nJ6LWXQt!uVND;NIu4Knl*X+O7`-{td1{k%G7POlKt
z<b1EtnZ*YdR(A_edae*!Qo82huS0YH{;8gzIq4i**ZK0!gqp`@H~%^NEm3kj_Sa*h
zoyOMVOjo?ZKdrV&kAHrz@%6K#!9Qoz3wqV5Oh3rP%*epFxN)aJ<2G=jl;vk+{LjL|
z%*4cEzz>XUSz!>L)qojD8OVYJ_*lePM9Lbcw-^1*-xnqsldq~GG;QVzPXPmYkhC(3
zgn?KCb_LdK9NKJ*tgP&ej4b*Fx(3=Xz5!#Kd`3x0ft9{~YDHphK~Ab(a(=FUGAMb+
z=O-5z=o=V_flOBgxrWDp3#O5o137g8^BpjCH8P}koLt}Ovq4!=A$D&%OUIi@#goqo
zmY+ZCZ#UU;0o%L&jcp?Ns_V|R={hnkyLii0?%cVXEnl>z>obIGb#J|FKPg4I`;opb
ztLE$e^{2N@_Ihu-<dvYN%<?~BclLXErM;K$o#{BsXYaCbmW=;zCY`Kb{zCLIZ)n0x
zfy8d_S??P}C13hidDXGJIOVn9OW~TJXyC;OrgBwzj#Hoh@(GSeefPgvW%-;xAqSqh
znm>BOz58ipvCp#oN4t7=UB2WTT0J%Bm-DK>@2_>TPIEZolVq?r$B|oTW$ZNuw!{6^
zaUz;Fd++p|4_H#Orh4-B4X4Ukl<(xfNiw&&!`#aw(DhGX4PWtnk;IlD)~dFvtJIg=
z71WcPca5z(E6X(2v{}mZTKVdElJ<&@lAVmo3_7p*?=@=`dgLDdbKuIuMpfJQ5t?$V
zrW$!Y>-@gEZ04ps%b=tynrr5D21m6BT<y32bjkZraQC|dmm9zEq$}pN{<V+%)28ha
WzrFIR(V5-W2Rp9#v+q?@HUt1PM)U*#

literal 0
HcmV?d00001

diff --git a/verifier/truststore/truststore.go b/verifier/truststore/truststore.go
index 067f5e63..067372c1 100644
--- a/verifier/truststore/truststore.go
+++ b/verifier/truststore/truststore.go
@@ -15,6 +15,7 @@
 package truststore
 
 import (
+	"bytes"
 	"context"
 	"crypto/x509"
 	"errors"
@@ -106,6 +107,14 @@ func (trustStore *x509TrustStore) GetCertificates(ctx context.Context, storeType
 		if err := ValidateCertificates(certs); err != nil {
 			return nil, CertificateError{InnerError: err, Msg: fmt.Sprintf("failed to validate the trusted certificate %s in trust store %s of type %s", certFileName, namedStore, storeType)}
 		}
+		// we require TSA certificates in trust store to be root CA certificates
+		if storeType == TypeTSA {
+			for _, cert := range certs {
+				if err := isRootCACertificate(cert); err != nil {
+					return nil, CertificateError{InnerError: err, Msg: fmt.Sprintf("trusted certificate %s in trust store %s of type %s is invalid: %v", certFileName, namedStore, storeType, err.Error())}
+				}
+			}
+		}
 		certificates = append(certificates, certs...)
 	}
 	if len(certificates) < 1 {
@@ -137,3 +146,14 @@ func ValidateCertificates(certs []*x509.Certificate) error {
 func isValidStoreType(storeType Type) bool {
 	return slices.Contains(Types, storeType)
 }
+
+// isRootCACertificate returns nil if cert is a root CA certificate
+func isRootCACertificate(cert *x509.Certificate) error {
+	if err := cert.CheckSignatureFrom(cert); err != nil {
+		return fmt.Errorf("certificate with subject %q is not a root CA certificate: %w", cert.Subject, err)
+	}
+	if !bytes.Equal(cert.RawSubject, cert.RawIssuer) {
+		return fmt.Errorf("certificate with subject %q is not a root CA certificate: issuer (%s) and subject (%s) are not the same", cert.Subject, cert.Issuer, cert.Subject)
+	}
+	return nil
+}
diff --git a/verifier/truststore/truststore_test.go b/verifier/truststore/truststore_test.go
index 762553e5..3ee3c740 100644
--- a/verifier/truststore/truststore_test.go
+++ b/verifier/truststore/truststore_test.go
@@ -98,3 +98,31 @@ func TestValidateCertsWithLeafCert(t *testing.T) {
 		t.Fatalf("leaf cert in a trust store should return error %q, got: %v", expectedErr, err)
 	}
 }
+
+func TestGetCertFromValidTsaTrustStore(t *testing.T) {
+	// testing ../testdata/truststore/x509/tsa/test-nonCA/globalsignRoot.cer
+	_, err := trustStore.GetCertificates(context.Background(), "tsa", "test-timestamp")
+	if err != nil {
+		t.Fatalf("expected nil error, but got %s", err)
+	}
+}
+
+func TestGetCertFromInvalidTsaTrustStore(t *testing.T) {
+	t.Run("non CA certificate", func(t *testing.T) {
+		// testing ../testdata/truststore/x509/tsa/test-nonCA/wabbit-networks.io
+		expectedErrMsg := `trusted certificate wabbit-networks.io.crt in trust store test-nonCA of type tsa is invalid: certificate with subject "CN=wabbit-networks.io,O=Notary,L=Seattle,ST=WA,C=US" is not a root CA certificate: x509: invalid signature: parent certificate cannot sign this kind of certificate`
+		_, err := trustStore.GetCertificates(context.Background(), "tsa", "test-nonCA")
+		if err == nil || err.Error() != expectedErrMsg {
+			t.Fatalf("expected error: %s, but got %s", expectedErrMsg, err)
+		}
+	})
+
+	t.Run("not self-issued", func(t *testing.T) {
+		//testing ../testdata/truststore/x509/tsa/test-nonSelfIssued/nonSelfIssued.crt
+		expectedErrMsg := `trusted certificate nonSelfIssued.crt in trust store test-nonSelfIssued of type tsa is invalid: certificate with subject "CN=Notation Test Revokable RSA Chain Cert 2,O=Notary,L=Seattle,ST=WA,C=US" is not a root CA certificate: issuer (CN=Notation Test Revokable RSA Chain Cert Root,O=Notary,L=Seattle,ST=WA,C=US) and subject (CN=Notation Test Revokable RSA Chain Cert 2,O=Notary,L=Seattle,ST=WA,C=US) are not the same`
+		_, err := trustStore.GetCertificates(context.Background(), "tsa", "test-nonSelfIssued")
+		if err == nil || err.Error() != expectedErrMsg {
+			t.Fatalf("expected error: %s, but got %s", expectedErrMsg, err)
+		}
+	})
+}