Implement an improved certificate manager #198
Conversation
ramondeklein
force-pushed
the
new-certs-manager
branch
from
09a253f to
ce08294
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements an improved certificate manager (Manager2) that addresses limitations in the existing certificate manager by adding support for directory scanning, certificate removal, proper resource cleanup, and SIGHUP signal handling for certificate rescanning.
Key changes:
- Introduced
Certificate2that automatically reloads certificates when underlying files change, with support for both regular files and symlinks - Implemented
Manager2that manages multiple certificates and reloads all certificates on SIGHUP signal - Added comprehensive test coverage for certificate auto-reloading, SIGHUP handling, and edge cases
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| certs/new_certificate.go | Implements Certificate2 with automatic file-watching and reloading capabilities |
| certs/new_certificate_test.go | Comprehensive tests for Certificate2 including auto-reload scenarios and error cases |
| certs/new_manager.go | Implements Manager2 with SIGHUP signal handling and certificate lifecycle management |
| certs/new_manager_test.go | Tests for Manager2 including Close, SIGHUP signal handling, and reload functionality |
Comments suppressed due to low confidence (1)
certs/new_manager.go:1
- Corrected grammar: 'Make sure the call Close' should be 'Make sure to call Close'.
// Copyright (c) 2015-2022 MinIO, Inc.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ramondeklein
force-pushed
the
new-certs-manager
branch
2 times, most recently
from
cd692c3 to
90c4ceb
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ramondeklein
force-pushed
the
new-certs-manager
branch
2 times, most recently
from
0d812a1 to
200c6d7
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ramondeklein
force-pushed
the
new-certs-manager
branch
from
285a67b to
100ea83
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
harshavardhana
left a comment
harshavardhana
left a comment
There was a problem hiding this comment.
What's with the naming of files as new_ ?
|
I named them |
harshavardhana
left a comment
harshavardhana
left a comment
There was a problem hiding this comment.
Overall approach looks good, will wait for @aead since he may better opinions
ramondeklein
force-pushed
the
new-certs-manager
branch
from
f4257d7 to
e29078b
Compare
ramondeklein
force-pushed
the
new-certs-manager
branch
from
218927e to
2a85ef6
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ramondeklein
force-pushed
the
new-certs-manager
branch
from
037d470 to
742b848
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ramondeklein
force-pushed
the
new-certs-manager
branch
2 times, most recently
from
cf67740 to
0c828a0
Compare
ramondeklein
force-pushed
the
new-certs-manager
branch
from
0c828a0 to
17881aa
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ramondeklein
force-pushed
the
new-certs-manager
branch
from
82d8949 to
cfe3231
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ramondeklein
force-pushed
the
new-certs-manager
branch
from
cfe3231 to
b1fc389
Compare
2 participants
Why a new certificate manager?
The existing certificate manager doesn't know how to rescan the manager to load all new and discard removed certificates. Since the old certificate manager doesn't know how to scan, this can't be done from the manager itself. This requires code -outside the manager- to listen to
SIGHUPand to update the certificates in the manager.There is a problem with the current manager, because it doesn't allow certificates to be removed. Also the underlying certificate code doesn't handle "unloading" them well. It leaks Go routines (in case the certificate is loaded via a symbolic link).
The new certificate manager implements the same functions, but during creation it is initialized using a "scanning" function that loads all the certificates. When
SIGHUPis received, then the same scanning function is invoked again to perform a full rescan.Global certificates
The EOS code uses HTTP clients that uses
GetCertificateorGetClientCertificate. This should ensure that the transport layer always uses the current certificate. Although this works, it creates a new certificate manager during the process and this will leak Go routines and file-system notification events.That's why
new_global_certs.gois introduced. It will use a certificate cache and will only load a certificate once (even if it's requested more often). This avoid leakage of Go routines and file-system notification events. It also simplifies the code...