Skip to content

use fromPEMFile in reponsiveness server#104

Merged
glbrntt merged 2 commits intoapple:mainfrom
rnro:use_fromPEMFile
Jul 24, 2025
Merged

use fromPEMFile in reponsiveness server#104
glbrntt merged 2 commits intoapple:mainfrom
rnro:use_fromPEMFile

Conversation

@rnro
Copy link
Copy Markdown
Contributor

@rnro rnro commented Jul 22, 2025

The previously used initializer is deprecated leading to warnings and CI failures

previously used API is deprecated

let certificate = try NIOSSLCertificate(file: certificatePath, format: .pem)
let certificate = try NIOSSLCertificate.fromPEMFile(certificatePath)
precondition(certificate.count == 1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
precondition(certificate.count == 1)
precondition(certificate.count >= 1)

The previous API ignored everything but the first certificate. In contrast this will fail if the file contains multiple certificates.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is true but at the moment the rest of the test is built assuming that there is only one certificate, we only pass along the first. The intention of this precondition is to reflect that and make it clear what is happening if we later change the file and we're ignoring elements of the certificate chain.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That we used NIOSSLCertificate(file:format:) here is a bug and the motivating reason for deprecating that API.

We should remove the precondition and pass along all certs to the certificateChain.

@rnro rnro requested a review from josephnoir July 22, 2025 13:12
@rnro rnro force-pushed the use_fromPEMFile branch from 5064bf7 to f099aeb Compare July 24, 2025 13:03
@rnro rnro requested a review from glbrntt July 24, 2025 13:28
@glbrntt glbrntt merged commit 492d99e into apple:main Jul 24, 2025
15 checks passed
@glbrntt glbrntt added the 🔨 semver/patch No public API change. label Jul 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🔨 semver/patch No public API change.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants