Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add TLS support for CTLog #1718

Merged
merged 5 commits into from
Jul 30, 2024
Merged

Conversation

fghanmi
Copy link
Contributor

@fghanmi fghanmi commented Jul 1, 2024

Summary

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

Release Note

  • Feature: Added support for TLS in communication with the CTLog.
  • 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: #1717

Documentation

Copy link

codecov bot commented Jul 1, 2024

Codecov Report

Attention: Patch coverage is 4.76190% with 20 lines in your changes missing coverage. Please review.

Project coverage is 50.21%. Comparing base (cf238ac) to head (3609bc5).
Report is 159 commits behind head on main.

Files Patch % Lines
cmd/app/serve.go 4.76% 20 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1718      +/-   ##
==========================================
- Coverage   57.93%   50.21%   -7.72%     
==========================================
  Files          50       70      +20     
  Lines        3119     4150    +1031     
==========================================
+ Hits         1807     2084     +277     
- Misses       1154     1837     +683     
- Partials      158      229      +71     

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

@fghanmi fghanmi force-pushed the CTlogTLSSupport branch from 58b8dab to efb5ae9 Compare July 1, 2024 12:38
@haydentherapper
Copy link
Contributor

Hey @fghanmi, sorry for the delay! Is there anything from sigstore/rekor#2164 that we should pull into this PR so this is in sync? Other question, is there any place we can add minimal tests?

cmd/app/serve.go Outdated
@@ -108,6 +109,7 @@ func newServeCmd() *cobra.Command {
cmd.Flags().String("grpc-tls-certificate", "", "the certificate file to use for secure connections - only applies to grpc-port")
cmd.Flags().String("grpc-tls-key", "", "the private key file to use for secure connections (without passphrase) - only applies to grpc-port")
cmd.Flags().Duration("idle-connection-timeout", 30*time.Second, "The time allowed for connections (HTTP or gRPC) to go idle before being closed by the server")
cmd.Flags().String("tls-ca-cert", "", "Path to TLS CA certificate")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should either scope this cmd line arg and/or update the help text to note that this only applies for connections to the ctlog

@fghanmi
Copy link
Contributor Author

fghanmi commented Jul 28, 2024

sigstore/rekor#2164
@haydentherapper,
I believe there is nothing to be added from sigstore/rekor#2164
Concerning the tests, I tried to run make up to docker-compose up, but the current configuration fails, I am not sure if I am running it wrong;
failed to load PEM certs file: open /etc/config/root.pem: permission denied

cmd/app/serve.go Outdated Show resolved Hide resolved
@haydentherapper
Copy link
Contributor

@fghanmi Hm, I didn't get such an error, I ran docker-compose build; docker-compose up. Do you want to try again?

@fghanmi
Copy link
Contributor Author

fghanmi commented Jul 29, 2024

@fghanmi Hm, I didn't get such an error, I ran docker-compose build; docker-compose up. Do you want to try again?

I had permissions issues on my host.. it's working fine now.


One more thing, I've noticed that ct_log server is not properly serving TLS when it's enabled. I've opened a small fix PR on certificate-transparency-go and when it's merged, I'll resume tests on this PR.

@haydentherapper
Copy link
Contributor

Sounds good, just ping this thread once you’ve tested it.

@fghanmi
Copy link
Contributor Author

fghanmi commented Jul 30, 2024

Sounds good, just ping this thread once you’ve tested it.

@haydentherapper, the update on certificate-transparency-go has been merged and I've tests the TLS communication between Fulcio and ct_server and it works fine now.

Inside Fulcio pod:

root@a899b97acafd:/go# curl  -v https://ct_server:6962/test --cacert /config/tls/ca.crt
*   Trying 192.168.64.7:6962...
* Connected to ct_server (192.168.64.7) port 6962 (#0)
* ALPN: offers h2,http/1.1
* TLSv1.3 (OUT), TLS handshake, Client hello (1):
*  CAfile: /config/tls/ca.crt
*  CApath: /etc/ssl/certs
* TLSv1.3 (IN), TLS handshake, Server hello (2):
* TLSv1.3 (IN), TLS handshake, Encrypted Extensions (8):
* TLSv1.3 (IN), TLS handshake, Certificate (11):
* TLSv1.3 (IN), TLS handshake, CERT verify (15):
* TLSv1.3 (IN), TLS handshake, Finished (20):
* TLSv1.3 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.3 (OUT), TLS handshake, Finished (20):
* SSL connection using TLSv1.3 / TLS_AES_128_GCM_SHA256
* ALPN: server accepted h2
* Server certificate:
*  subject: OU=Server; CN=*; emailAddress=tls@gmail.com
*  start date: Jul 30 18:59:47 2024 GMT
*  expire date: Sep 28 18:59:47 2024 GMT
*  subjectAltName: host "ct_server" matched cert's "ct_server"
*  issuer: CN=My CA
*  SSL certificate verify ok.
* using HTTP/2
* h2h3 [:method: GET]
* h2h3 [:path: /test]
* h2h3 [:scheme: https]
* h2h3 [:authority: ct_server:6962]
* h2h3 [user-agent: curl/7.88.1]
* h2h3 [accept: */*]
* Using Stream ID: 1 (easy handle 0x557648bc0c80)
> GET /test HTTP/2
> Host: ct_server:6962
> user-agent: curl/7.88.1
> accept: */*
> 
* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
< HTTP/2 404 
< vary: Origin
< content-length: 0
< date: Tue, 30 Jul 2024 19:54:48 GMT
< 
* Connection #0 to host ct_server left intact

Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Can you resolve the conflict as well?

docker-compose.yml Show resolved Hide resolved
fghanmi added 5 commits July 30, 2024 23:58
Signed-off-by: Firas Ghanmi <fghanmi@redhat.com>
Signed-off-by: Firas Ghanmi <fghanmi@redhat.com>
Signed-off-by: Firas Ghanmi <fghanmi@redhat.com>
Signed-off-by: Firas Ghanmi <fghanmi@redhat.com>
Signed-off-by: Firas Ghanmi <fghanmi@redhat.com>
@haydentherapper haydentherapper enabled auto-merge (squash) July 30, 2024 22:24
@haydentherapper haydentherapper merged commit 501c5fd into sigstore:main Jul 30, 2024
13 checks passed
fghanmi added a commit to securesign/fulcio that referenced this pull request Jul 31, 2024
* Add TLS support for CTLog

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

* update tls-ca-cert cmd line

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

* updates

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

* update TLS certificates

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

* fix conflicts, add keys generation commands

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

---------

Signed-off-by: Firas Ghanmi <fghanmi@redhat.com>
fghanmi added a commit to securesign/fulcio that referenced this pull request Jul 31, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants