Skip to content

Conversation

@gautierdelorme
Copy link
Contributor

Support loading RSA PSS public keys with parameters.

Checklist

  • I've run tests to see all new and existing tests pass
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

If you've made changes to gyb files

  • I've run .script/generate_boilerplate_files_with_gyb and included updated generated files in a commit of this pull request

Motivation:

Trying to load those keys currently fails.

Modifications:

Parameters are stripped and the key is treated as a regular public key.

Result:

_RSA.Signing.PublicKey() works with RSA PSS keys with parameters.

Parameters are stripped and the key is treated as a regular public key.
@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Oct 3, 2024
@Lukasa
Copy link
Contributor

Lukasa commented Oct 3, 2024

@swift-server-bot add to allowlist

Package.swift Outdated
],
dependencies: [],
dependencies: [
.package(url: "https://github.com/apple/swift-asn1.git", .upToNextMajor(from: "1.2.0"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

Suggested change
.package(url: "https://github.com/apple/swift-asn1.git", .upToNextMajor(from: "1.2.0"))
.package(url: "https://github.com/apple/swift-asn1.git", from: "1.2.0")

//
//
// Created by Gautier Delorme on 24/09/2024.
//
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add the license header that is present in other files and remove this header?

/// - Warning: Key sizes less than 2048 are not recommended and should only be used for compatibility reasons.
public init<Bytes: DataProtocol>(unsafeDERRepresentation derRepresentation: Bytes) throws {
self.backing = try BackingPublicKey(derRepresentation: derRepresentation)
let sanitizedDer = try SubjectPublicKeyInfo.stripRsaPssParameters(derEncoded: [UInt8](derRepresentation))
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a request for changes in this PR, but can you file an issue that asks us to move all our RSA key parsing into Swift ASN1? This is a pragmatic change for now, but as we're already parsing the key we should probably just stick with that now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Lukasa
Copy link
Contributor

Lukasa commented Oct 3, 2024

Thanks for this PR! I've left a few notes in the diff for cleaning up.

return derEncoded
}

if spki.algorithmIdentifier.algorithm == .AlgorithmIdentifier.rsaPSS {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we turn this into a shortcut early return? If we don't have to make any modifications, we don't need to re-encode the key.

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Cool, this LGTM.

@Lukasa
Copy link
Contributor

Lukasa commented Oct 3, 2024

Thanks very much!

@Lukasa Lukasa enabled auto-merge (squash) October 3, 2024 17:23
auto-merge was automatically disabled October 3, 2024 17:23

Head branch was pushed to by a user without write access

@Lukasa Lukasa merged commit b639b5b into apple:main Oct 3, 2024
@gautierdelorme gautierdelorme deleted the gdelorme/rsapss branch October 3, 2024 17:41
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.

2 participants