-
-
Notifications
You must be signed in to change notification settings - Fork 5k
docs(CertificateManager/ICertificateManager): clarify API #60363
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
Open
joshtrichards
wants to merge
4
commits into
master
Choose a base branch
from
jtr/docs-CertificateManager
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
b5e3b96
docs(security): clarify CertificateManager bundle lifecycle
joshtrichards 27cc429
docs(security): clarify ICertificateManager API and certificate handling
joshtrichards 0a19290
chore(CertificateManager): drop docs for methods covered in interface
joshtrichards 6a2d392
chore(CertificateManager): fixup for lint
joshtrichards File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,9 +15,6 @@ | |
| use OCP\Security\ISecureRandom; | ||
| use Psr\Log\LoggerInterface; | ||
|
|
||
| /** | ||
| * Manage trusted certificates for users | ||
| */ | ||
| class CertificateManager implements ICertificateManager { | ||
| private ?string $bundlePath = null; | ||
|
|
||
|
|
@@ -29,11 +26,6 @@ public function __construct( | |
| ) { | ||
| } | ||
|
|
||
| /** | ||
| * Returns all certificates trusted by the user | ||
| * | ||
| * @return ICertificate[] | ||
| */ | ||
| #[\Override] | ||
| public function listCertificates(): array { | ||
| if (!$this->config->getSystemValueBool('installed', false)) { | ||
|
|
@@ -67,6 +59,9 @@ public function listCertificates(): array { | |
| return $result; | ||
| } | ||
|
|
||
| /** | ||
| * Check whether any uploaded certificates are present. | ||
| */ | ||
| private function hasCertificates(): bool { | ||
| if (!$this->config->getSystemValueBool('installed', false)) { | ||
| return false; | ||
|
|
@@ -91,9 +86,14 @@ private function hasCertificates(): bool { | |
| } | ||
|
|
||
| /** | ||
| * create the certificate bundle of all trusted certificated | ||
| * Rebuild the generated effected certificate bundle from: | ||
| * - uploaded certificates | ||
| * - the shipped default CA bundle | ||
| * - the current system CA bundle, if present and different from the target | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either this is not true or I don't understand what you mean. Nextcloud never uses the system CA bundle. |
||
| * | ||
| * The bundle is written atomically to /files_external/rootcerts.crt. | ||
| */ | ||
| public function createCertificateBundle(): void { | ||
| private function createCertificateBundle(): void { | ||
| $path = $this->getPathToCertificates(); | ||
| $certs = $this->listCertificates(); | ||
|
|
||
|
|
@@ -142,13 +142,6 @@ public function createCertificateBundle(): void { | |
| $this->view->rename($tmpPath, $certPath); | ||
| } | ||
|
|
||
| /** | ||
| * Save the certificate and re-generate the certificate bundle | ||
| * | ||
| * @param string $certificate the certificate data | ||
| * @param string $name the filename for the certificate | ||
| * @throws \Exception If the certificate could not get added | ||
| */ | ||
| #[\Override] | ||
| public function addCertificate(string $certificate, string $name): ICertificate { | ||
| $path = $this->getPathToCertificates() . 'uploads/' . $name; | ||
|
|
@@ -171,9 +164,6 @@ public function addCertificate(string $certificate, string $name): ICertificate | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Remove the certificate and re-generate the certificate bundle | ||
| */ | ||
| #[\Override] | ||
| public function removeCertificate(string $name): bool { | ||
| $path = $this->getPathToCertificates() . 'uploads/' . $name; | ||
|
|
@@ -192,18 +182,11 @@ public function removeCertificate(string $name): bool { | |
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * Get the path to the certificate bundle | ||
| */ | ||
| #[\Override] | ||
| public function getCertificateBundle(): string { | ||
| return $this->getPathToCertificates() . 'rootcerts.crt'; | ||
| } | ||
|
|
||
| /** | ||
| * Get the full local path to the certificate bundle | ||
| * @throws \Exception when getting bundle path fails | ||
| */ | ||
| #[\Override] | ||
| public function getAbsoluteBundlePath(): string { | ||
| try { | ||
|
|
@@ -230,12 +213,20 @@ public function getAbsoluteBundlePath(): string { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Get the base path used to store uploaded certificates and the generated bundle. | ||
| * | ||
| * The uploaded certificates and generated bundle are stored under the | ||
| * files_external path for historical reasons, maintaining compatibility | ||
| * with pre-existing deployments. | ||
| */ | ||
| private function getPathToCertificates(): string { | ||
| return '/files_external/'; | ||
| } | ||
|
|
||
| /** | ||
| * Check if we need to re-bundle the certificates because one of the sources has updated | ||
| * Determine whether the generated bundle must be rebuilt because the source | ||
| * CA bundle has changed or the target bundle is missing. | ||
| */ | ||
| private function needsRebundling(): bool { | ||
| $targetBundle = $this->getCertificateBundle(); | ||
|
|
@@ -248,7 +239,7 @@ private function needsRebundling(): bool { | |
| } | ||
|
|
||
| /** | ||
| * get mtime of ca-bundle shipped by Nextcloud | ||
| * Return the modification time of the shipped default CA bundle. | ||
| */ | ||
| protected function getFilemtimeOfCaBundle(): int { | ||
| return filemtime($this->getDefaultCertificatesBundlePath()); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.