Add TLS support for Trillian server #2164

fghanmi · 2024-06-30T20:32:40Z

Summary

This pull request introduces support for enabling TLS in communications with the Trillian server. By adding a new command-line flag --tls-ca-cert and implementing the necessary logic to handle TLS certificates, this update enhances the security of Rekor.

Release Note

Feature: Added support for TLS in communication with the Trillian server.
New Flag: --tls-ca-cert to specify the CA certificate file path for secure connections.
Behavior: If the --tls-ca-cert flag is not provided, the system will default to insecure connections.
Security: This update significantly enhances the security of data in transit by enabling TLS.

Resolves Issue: #2163

Documentation

codecov · 2024-07-01T10:53:07Z

Codecov Report

Attention: Patch coverage is 8.00000% with 23 lines in your changes missing coverage. Please review.

Project coverage is 43.23%. Comparing base (488eb97) to head (c1f5a69).
Report is 166 commits behind head on main.

Files	Patch %	Lines
pkg/api/api.go	0.00%	23 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2164       +/-   ##
===========================================
- Coverage   66.46%   43.23%   -23.23%     
===========================================
  Files          92      188       +96     
  Lines        9258    19368    +10110     
===========================================
+ Hits         6153     8374     +2221     
- Misses       2359    10238     +7879     
- Partials      746      756       +10

Flag	Coverage Δ
e2etests	`?`
unittests	`43.23% <8.00%> (-4.45%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cpanato

lgtm

cmd/rekor-server/app/root.go

pkg/api/api.go

bobcallaway

lgtm minus lint issue, thanks for adding this!

cmd/rekor-server/app/root.go

bobcallaway · 2024-07-03T09:29:43Z

tests/tls/ca.crt

@@ -0,0 +1,29 @@
+-----BEGIN CERTIFICATE-----


Not Before: Jul 2 22:26:18 2024 GMT Not After : Aug 31 22:26:18 2024 GMT

Can we either make this expire quite far out in the future or generate the cert ephemerally on each test run?

bobcallaway · 2024-07-03T09:33:52Z

pkg/api/api.go

+	switch {
+	case useSystemTrustStore:
+		creds = credentials.NewTLS(&tls.Config{
+			ServerName: rpcServer,


it's unlikely, but possible that rpcServer will not simply be a hostname/IP address or hostname:port combination, as that string can be a value as defined by https://github.com/grpc/grpc/blob/master/doc/naming.md

pkg/api/api.go

fghanmi · 2024-07-03T09:56:03Z

@bobcallaway I see that the e2e tests are failing, are they all using ./docker-compose.yaml resources ?
Meaning, should I enable TLS on all the /docker-compose*.yaml files ? ( since the trillian logserver in docker-compose.yaml is TLS enabled)

fghanmi · 2024-07-03T13:21:00Z

Regarding the build failure: CI / issue-872-e2e:
The script is using an old rekor-server image https://github.com/sigstore/rekor/blob/main/tests/issue-872-e2e-test.sh#L71
that does not have the new tag --tls_ca_cert and thus, it fails.
@bobcallaway is it possible to use an image with the new updates ?

bobcallaway · 2024-07-03T15:06:43Z

Regarding the build failure: CI / issue-872-e2e: The script is using an old rekor-server image https://github.com/sigstore/rekor/blob/main/tests/issue-872-e2e-test.sh#L71 that does not have the new tag --tls_ca_cert and thus, it fails. @bobcallaway is it possible to use an image with the new updates ?

no, that specific test is for a regression that was started at that back version.

I'll need to take a closer look at the dependency structure of those various docker-compose files to better advise you.

fghanmi · 2024-07-07T13:52:09Z

Regarding the build failure: CI / issue-872-e2e: The script is using an old rekor-server image https://github.com/sigstore/rekor/blob/main/tests/issue-872-e2e-test.sh#L71 that does not have the new tag --tls_ca_cert and thus, it fails. @bobcallaway is it possible to use an image with the new updates ?

no, that specific test is for a regression that was started at that back version.

I'll need to take a closer look at the dependency structure of those various docker-compose files to better advise you.

We can create another trillian server trillian-log-server-no-tls and use it as the trillian server for the rekor-server-issue-872-v060 that uses an older image. (I've tested it)
What do you think ?

bobcallaway · 2024-07-23T12:34:35Z

Let's do this for now; remove the test coverage from this PR and please open an issue on the repo to add back coverage later.

fghanmi · 2024-07-23T12:54:26Z

Let's do this for now; remove the test coverage from this PR and please open an issue on the repo to add back coverage later.

Done.
Issue created: #2188

Signed-off-by: Firas Ghanmi <fghanmi@redhat.com>

cmd/rekor-server/app/root.go

Signed-off-by: Firas Ghanmi <fghanmi@redhat.com>

* Add TLS support for Trillian server Signed-off-by: Firas Ghanmi <fghanmi@redhat.com> * update tls_ca_cert key name Signed-off-by: Firas Ghanmi <fghanmi@redhat.com> --------- Signed-off-by: Firas Ghanmi <fghanmi@redhat.com>

#### Summary This pull request introduces support for enabling TLS in communications with the Trillian server. By adding a new command-line flag `--trillian_log_server.tls_ca_cert` and implementing the necessary logic to handle TLS certificates, this update enhances the security of Rekor. #### Release Note - Feature: Added support for TLS in communication with the Trillian server. - New Flag: - `--trillian_log_server.tls_ca_cert` to specify the CA certificate file path for secure connections. Resolves Issue: sigstore#2163 --------- Signed-off-by: Firas Ghanmi <fghanmi@redhat.com>

fghanmi requested a review from a team as a code owner June 30, 2024 20:32

fghanmi force-pushed the TrillianTLSSupport branch from a5bc1cc to 6cdb8d3 Compare June 30, 2024 20:36

fghanmi force-pushed the TrillianTLSSupport branch from 6cdb8d3 to 9818847 Compare July 1, 2024 12:11

cpanato previously approved these changes Jul 2, 2024

View reviewed changes

cpanato requested a review from haydentherapper July 2, 2024 10:20

bobcallaway reviewed Jul 2, 2024

View reviewed changes

cmd/rekor-server/app/root.go Outdated Show resolved Hide resolved

pkg/api/api.go Outdated Show resolved Hide resolved

cpanato self-requested a review July 2, 2024 13:58

fghanmi dismissed cpanato’s stale review via 06ad4f3 July 2, 2024 15:48

fghanmi force-pushed the TrillianTLSSupport branch 2 times, most recently from 06ad4f3 to b807792 Compare July 2, 2024 15:49

fghanmi requested a review from bobcallaway July 2, 2024 15:53

bobcallaway reviewed Jul 2, 2024

View reviewed changes

cmd/rekor-server/app/root.go Outdated Show resolved Hide resolved

fghanmi force-pushed the TrillianTLSSupport branch 4 times, most recently from 9679d43 to bc916fa Compare July 2, 2024 22:29

fghanmi requested a review from bobcallaway July 3, 2024 06:44

bobcallaway reviewed Jul 3, 2024

View reviewed changes

pkg/api/api.go Outdated Show resolved Hide resolved

fghanmi force-pushed the TrillianTLSSupport branch from bc916fa to bacd9af Compare July 3, 2024 09:43

fghanmi force-pushed the TrillianTLSSupport branch from bacd9af to 6df12e8 Compare July 3, 2024 10:36

fghanmi requested a review from bobcallaway July 3, 2024 10:38

fghanmi force-pushed the TrillianTLSSupport branch from 6df12e8 to e665262 Compare July 3, 2024 11:06

fghanmi mentioned this pull request Jul 23, 2024

Add TLS test coverage #2188

Open

Add TLS support for Trillian server

4903305

Signed-off-by: Firas Ghanmi <fghanmi@redhat.com>

fghanmi force-pushed the TrillianTLSSupport branch from faf01b7 to 4903305 Compare July 23, 2024 13:21

bobcallaway reviewed Jul 23, 2024

View reviewed changes

cmd/rekor-server/app/root.go Outdated Show resolved Hide resolved

update tls_ca_cert key name

c1f5a69

Signed-off-by: Firas Ghanmi <fghanmi@redhat.com>

bobcallaway approved these changes Jul 23, 2024

View reviewed changes

bobcallaway enabled auto-merge (squash) July 23, 2024 21:06

haydentherapper mentioned this pull request Jul 23, 2024

Add TLS support for CTLog sigstore/fulcio#1718

Merged

bobcallaway merged commit 5e502df into sigstore:main Jul 23, 2024
15 checks passed

github-actions bot added this to the v1.2.2 milestone Jul 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TLS support for Trillian server #2164

Add TLS support for Trillian server #2164

fghanmi commented Jun 30, 2024 •

edited

Loading

codecov bot commented Jul 1, 2024 •

edited

Loading

cpanato left a comment

bobcallaway left a comment

bobcallaway Jul 3, 2024

bobcallaway Jul 3, 2024

fghanmi commented Jul 3, 2024 •

edited

Loading

fghanmi commented Jul 3, 2024

bobcallaway commented Jul 3, 2024

fghanmi commented Jul 7, 2024 •

edited

Loading

bobcallaway commented Jul 23, 2024

fghanmi commented Jul 23, 2024

Add TLS support for Trillian server #2164

Add TLS support for Trillian server #2164

Conversation

fghanmi commented Jun 30, 2024 • edited Loading

Summary

Release Note

Documentation

codecov bot commented Jul 1, 2024 • edited Loading

Codecov Report

cpanato left a comment

Choose a reason for hiding this comment

bobcallaway left a comment

Choose a reason for hiding this comment

bobcallaway Jul 3, 2024

Choose a reason for hiding this comment

bobcallaway Jul 3, 2024

Choose a reason for hiding this comment

fghanmi commented Jul 3, 2024 • edited Loading

fghanmi commented Jul 3, 2024

bobcallaway commented Jul 3, 2024

fghanmi commented Jul 7, 2024 • edited Loading

bobcallaway commented Jul 23, 2024

fghanmi commented Jul 23, 2024

fghanmi commented Jun 30, 2024 •

edited

Loading

codecov bot commented Jul 1, 2024 •

edited

Loading

fghanmi commented Jul 3, 2024 •

edited

Loading

fghanmi commented Jul 7, 2024 •

edited

Loading