Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add TLS support for migrillian #1525

Merged
merged 1 commit into from
Jul 4, 2024

Conversation

fghanmi
Copy link
Contributor

@fghanmi fghanmi commented Jul 1, 2024

Summary

This pull request introduces the option to specify a CA certificate for establishing secure connections with the Trillian server.
By using --trillian_tls_ca_cert_file flag, users can provide a CA certificate, that is used to establish a secure communication with Trillian server.

Release Note

  • Feature: Added support for TLS security for Trillian server.
    New Flag: --trillian_tls_ca_cert_file to specify the file path to the CA certificate.
    Behavior: If --trillian_tls_ca_cert_file 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: #1524

Checklist

@fghanmi fghanmi requested a review from a team as a code owner July 1, 2024 09:36
@fghanmi fghanmi requested review from mhutchinson and removed request for a team July 1, 2024 09:36
@fghanmi
Copy link
Contributor Author

fghanmi commented Jul 1, 2024

Hello,
It seems that "Missing permissions: cloudbuild.builds.get" prevents the build from starting/succeeding.
Could you please help ?
Thanks!

@fghanmi fghanmi force-pushed the TrillianTLSSupport branch from f1e140e to 4b960ec Compare July 1, 2024 12:42
Copy link
Contributor

@mhutchinson mhutchinson left a comment

Choose a reason for hiding this comment

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

Can you clarify in the PR description that this support is only for TLS certs signed by a CA. This does not add support for self-signed TLS certs.

trillian/migrillian/main.go Outdated Show resolved Hide resolved
trillian/migrillian/main.go Outdated Show resolved Hide resolved
trillian/migrillian/main.go Show resolved Hide resolved
trillian/migrillian/main.go Show resolved Hide resolved
trillian/migrillian/main.go Outdated Show resolved Hide resolved
@fghanmi fghanmi force-pushed the TrillianTLSSupport branch 2 times, most recently from 7b95849 to 9593986 Compare July 3, 2024 15:00
@fghanmi
Copy link
Contributor Author

fghanmi commented Jul 3, 2024

Can you clarify in the PR description that this support is only for TLS certs signed by a CA. This does not add support for self-signed TLS certs.

CA certificate can be self-generated. Here, we only need the CA certificate to validate Trillian's TLS certificates to establish the secure communication.
The TLS certs signed by a CA (or self-signed TLS certs) will be used to secure CT log server (in the other issue)

@fghanmi fghanmi requested a review from mhutchinson July 3, 2024 15:30
@mhutchinson
Copy link
Contributor

Can you clarify in the PR description that this support is only for TLS certs signed by a CA. This does not add support for self-signed TLS certs.

CA certificate can be self-generated. Here, we only need the CA certificate to validate Trillian's TLS certificates to establish the secure communication. The TLS certs signed by a CA (or self-signed TLS certs) will be used to secure CT log server (in the other issue)

Don't worry about it. Thinking about this some more, the self-signed cert method will still work using this approach as far as I can see because the end-cert is also its own root cert.

@mhutchinson
Copy link
Contributor

/gcbrun

@fghanmi
Copy link
Contributor Author

fghanmi commented Jul 3, 2024

/gcbrun

I've checked the govulncheck / Run govulncheck (pull_request) error, and it seems, master does have the same issue.

@mhutchinson
Copy link
Contributor

/gcbrun

I've checked the govulncheck / Run govulncheck (pull_request) error, and it seems, master does have the same issue.

Yeah that's because govulncheck isn't hermetic and so the master branch fails now, even though it passed when it was last updated. I'll try to get a PR in that fixes it 👍

@mhutchinson
Copy link
Contributor

#1527

@roger2hk
Copy link
Contributor

roger2hk commented Jul 3, 2024

@fghanmi #1527 has been merged. Please rebase the branch.

@fghanmi fghanmi force-pushed the TrillianTLSSupport branch from 9593986 to ec708d9 Compare July 3, 2024 18:27
@mhutchinson
Copy link
Contributor

I was just about to merge this and noticed that you haven't updated the CHANGELOG file. Can you copy in the notes from this PR description into the CHANGELOG.md file? Thanks!

Signed-off-by: Firas Ghanmi <fghanmi@redhat.com>
@fghanmi fghanmi force-pushed the TrillianTLSSupport branch from ec708d9 to c270698 Compare July 4, 2024 09:31
@mhutchinson
Copy link
Contributor

/gcbrun

@mhutchinson mhutchinson merged commit a549614 into google:master Jul 4, 2024
6 checks passed
fghanmi added a commit to securesign/certificate-transparency-go that referenced this pull request Jul 15, 2024
Signed-off-by: Firas Ghanmi <fghanmi@redhat.com>
@roger2hk roger2hk changed the title Add TLS support for Trillian server Add TLS support for migrillian Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants