Skip to content

docs(CertificateManager/ICertificateManager): clarify API - #60363

Open
joshtrichards wants to merge 7 commits into
masterfrom
jtr/docs-CertificateManager
Open

docs(CertificateManager/ICertificateManager): clarify API#60363
joshtrichards wants to merge 7 commits into
masterfrom
jtr/docs-CertificateManager

Conversation

@joshtrichards

@joshtrichards joshtrichards commented May 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Member
  • Resolves: #

Summary

Clarify and align the CertificateManager implementation and ICertificateManager public contract around uploaded certificates, generated bundles, and the configured default CA bundle.

This also removes obsolete code from the former per-user certificate-bundle model, which was removed in #21693.

Changes

  • Document that uploaded certificates are combined with the configured default CA bundle, which defaults to Nextcloud's shipped CA bundle.
  • Clarify that getCertificateBundle() returns a virtual filesystem path and that getAbsoluteBundlePath() may fall back to the configured default CA bundle.
  • Make the internal CertificateManager::createCertificateBundle() helper private. It was already outside the interface and is only used internally.
  • Remove the unreachable append of the former separate Nextcloud-wide bundle from createCertificateBundle().
  • Fix the certificate-header check so PEM data beginning at offset zero is included in generated bundles.
  • Simplify minor related implementation details.
  • Update the config.sample.php entry for default_certificates_bundle_path for alignment and improved clarity.

TODO

  • Possibly make one more pass back through the implementation to drop some bits now better covered in the interface docs

Checklist

AI (if applicable)

  • The content of this PR was partly or fully generated using AI

@joshtrichards
docs(security): clarify CertificateManager bundle lifecycle
b5e3b96 
Signed-off-by: Josh <josh.t.richards@gmail.com>
@joshtrichards
docs(security): clarify ICertificateManager API and certificate handling
27cc429 
Signed-off-by: Josh <josh.t.richards@gmail.com>
@joshtrichards joshtrichards changed the title docs(CertificateManager): clarify ICertificateManager/CertificateManager APIs/interface docs(CertificateManager): clarify ICertificateManager/CertificateManager API May 13, 2026
@joshtrichards joshtrichards changed the title docs(CertificateManager): clarify ICertificateManager/CertificateManager API docs(CertificateManager/ICertificateManager): clarify API May 13, 2026
@joshtrichards
joshtrichards marked this pull request as ready for review May 13, 2026 18:47
@joshtrichards
joshtrichards requested a review from a team as a code owner May 13, 2026 18:47
@joshtrichards joshtrichards added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 13, 2026
@joshtrichards
joshtrichards requested review from CarlSchwan, artonge, blizzz, icewind1991, leftybournes, provokateurin and salmart-dev and removed request for a team May 13, 2026 18:47
provokateurin
provokateurin reviewed May 18, 2026
View reviewed changes
Comment thread lib/private/Security/CertificateManager.php
@joshtrichards
chore(CertificateManager): drop docs for methods covered in interface
0a19290 
Signed-off-by: Josh <josh.t.richards@gmail.com>
@joshtrichards joshtrichards added this to the Nextcloud 35 milestone May 18, 2026
@joshtrichards
chore(CertificateManager): fixup for lint
6a2d392 
Signed-off-by: Josh <josh.t.richards@gmail.com>
provokateurin
provokateurin reviewed May 19, 2026
View reviewed changes
Comment thread lib/private/Security/CertificateManager.php Outdated
* 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 reviewed Jun 8, 2026
View reviewed changes
Comment thread lib/private/Security/CertificateManager.php Outdated
* 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

@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

@susnux susnux added the community pull requests from community label Jun 9, 2026
@joshtrichards
fix(security): clean up CertificateManager bundle handling
64aedd4 
Remove the obsolete system-wide bundle append left over from the former per-user certificate-bundle model. Since #21693 consolidated certificate handling to a single Nextcloud-wide bundle, the append condition can no longer be satisfied.

Clarify the implementation terminology around uploaded certificates and the configured default CA bundle, and remove redundant exception handling in addCertificate(). Also close directory handles on all paths and use str_contains() when checking uploaded certificate contents so a PEM header at offset zero is handled correctly.

Signed-off-by: Josh <josh.t.richards@gmail.com>
@joshtrichards
docs(ocp): clarify ICertificateManager contract
bc6d031 
Document the distinction between uploaded trusted certificates, the
generated certificate bundle, and the configured default CA bundle.

Clarify that listCertificates() returns only uploaded certificates,
getCertificateBundle() returns a virtual filesystem path, and getAbsoluteBundlePath() may fall back to the configured default CA bundle. Remove the obsolete reference to a separate system bundle being appended.

Signed-off-by: Josh <josh.t.richards@gmail.com>
@joshtrichards
docs(config): clarify default CA bundle override
372ae2d 
Document that default_certificates_bundle_path overrides Nextcloud's shipped CA bundle used to verify TLS certificates.

Clarify that the configured value must reference a readable local CA
bundle, is used directly when no certificates are uploaded, and is
included when Nextcloud generates the bundle containing uploaded
certificates. Simplify the default value and use the Debian-derived system CA bundle path as the configuration-sample example for further clarity.

Signed-off-by: Josh <josh.t.richards@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@salmart-dev salmart-dev Awaiting requested review from salmart-dev salmart-dev is a code owner automatically assigned from nextcloud/server-backend
@artonge artonge Awaiting requested review from artonge artonge is a code owner automatically assigned from nextcloud/server-backend
@leftybournes leftybournes Awaiting requested review from leftybournes leftybournes is a code owner automatically assigned from nextcloud/server-backend
@icewind1991 icewind1991 Awaiting requested review from icewind1991
@blizzz blizzz Awaiting requested review from blizzz
@provokateurin provokateurin Awaiting requested review from provokateurin
@CarlSchwan CarlSchwan Awaiting requested review from CarlSchwan

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

3. to review Waiting for reviews community pull requests from community developer experience enhancement security

Projects

None yet

Milestone

Nextcloud 35

Development

Successfully merging this pull request may close these issues.

4 participants

@joshtrichards @CarlSchwan @provokateurin @susnux