Skip to content

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.

Sign up for GitHub

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 7 commits into
base: master
Choose a base branch
from
Open

docs(CertificateManager/ICertificateManager): clarify API #60363

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b5e3b96
docs(security): clarify CertificateManager bundle lifecycle
joshtrichards May 13, 2026
27cc429
docs(security): clarify ICertificateManager API and certificate handling
joshtrichards May 13, 2026
0a19290
chore(CertificateManager): drop docs for methods covered in interface
joshtrichards May 18, 2026
6a2d392
chore(CertificateManager): fixup for lint
joshtrichards May 18, 2026
64aedd4
fix(security): clean up CertificateManager bundle handling
joshtrichards Jul 28, 2026
bc6d031
docs(ocp): clarify ICertificateManager contract
joshtrichards Jul 28, 2026
372ae2d
docs(config): clarify default CA bundle override
joshtrichards Jul 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 20 additions & 29 deletions lib/private/Security/CertificateManager.php
View file
Open in desktop
Comment thread
provokateurin marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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)) {
Expand Down Expand Up @@ -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;
Expand All @@ -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

@provokateurin provokateurin May 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.

@joshtrichards joshtrichards Jul 28, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch. I traced this further and realized I had confused two different things (and inadvertently also discovered some dead code):

server/lib/private/Security/CertificateManager.php

Lines 133 to 138 in 6a2d392

// Append the system certificate bundle
$systemBundle = $this->getCertificateBundle();
if ($systemBundle !== $certPath && $this->view->file_exists($systemBundle)) {
$systemCertificates = $this->view->file_get_contents($systemBundle);
fwrite($fhCerts, $systemCertificates);
}

  1. The existing comment on line 133 refers to the Nextcloud system-wide bundle, not the host system CA bundle.
  2. This bit of code is a relic of the formerly supported per-user bundle model (which was dropped in Improve CertificateManager to not be user context dependent #21693).
  3. Since Improve CertificateManager to not be user context dependent #21693, the code in the conditional that follows that comment is dead code.

I’ll remove the inaccurate wording from this PR, along with the stale comment and unreachable code in the implementation.

@CarlSchwan CarlSchwan Jun 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
* - the current system CA bundle, if present and different from the target

*
* The bundle is written atomically to /files_external/rootcerts.crt.
*/
public function createCertificateBundle(): void {
private function createCertificateBundle(): void {
$path = $this->getPathToCertificates();
$certs = $this->listCertificates();

Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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 {
Expand All @@ -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();
Expand All @@ -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());
Expand Down
33 changes: 24 additions & 9 deletions lib/public/ICertificateManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,52 +9,67 @@
namespace OCP;

/**
* Manage trusted certificates
* Manage trusted certificates and the effective CA bundle used by Nextcloud.
*
* Implementations provide access to uploaded trusted certificates and the
* generated bundle that is consumed by HTTP clients and external storage
* integrations.
*
* @since 8.0.0
*/
interface ICertificateManager {
/**
* Returns all certificates trusted by the system
* Returns all uploaded trusted certificates.
*
* This does not include the shipped default CA bundle or any system CA bundle
* appended when building the effective bundle.
*
* @return \OCP\ICertificate[]
* @since 8.0.0
*/
public function listCertificates(): array;

/**
* @param string $certificate the certificate data
* @param string $name the filename for the certificate
* Add a trusted certificate to the certificate store.
*
* @param string $certificate The certificate data in PEM format
* @param string $name The filename for the certificate
* @return \OCP\ICertificate
* @throws \Exception If the certificate could not get added
* @throws \Exception If the certificate could not be added
* @since 8.0.0 - since 8.1.0 throws exception instead of returning false
*/
public function addCertificate(string $certificate, string $name): \OCP\ICertificate;

/**
* @param string $name
* Remove a trusted certificate from the certificate store.
*
* @param string $name The filename for the certificate
* @return bool
* @since 8.0.0
*/
public function removeCertificate(string $name): bool;

/**
* Get the path to the certificate bundle
* Get the relative path to the generated certificate bundle.
*
* @return string
* @since 8.0.0
*/
public function getCertificateBundle(): string;

/**
* Get the full local path to the certificate bundle
* Get the full local path to the effective certificate bundle.
*
* Implementations should return the generated bundle path, but may log and fall back
* to the shipped default CA bundle if resolution fails.
*
* @return string
* @since 9.0.0
*/
public function getAbsoluteBundlePath(): string;

/**
* Get the path of the default certificates bundle.
* Get the path of the shipped default certificates bundle.
*
* @since 33.0.0
*/
Expand Down
Loading