Inconsistency with password behavior between load_pem_private_key() and load_ssh_private_key()

[hannob]

I noticed that the behavior regarding password protected keys between load_pem_private_key() and load_ssh_private_key() is somewhat inconsistent.

load_pem_private_key() will throw a TypeError if no or a wrong password is given for a protected key or if a password is given for a key without a password.

load_ssh_private_key() will throw a ValueError in the first case, and just import the key if it's unprotected and a password is given.

I'm aware changing it would be an API-breaking change and may impact existing applications, so not sure you want to fix this. Consider it a feature request, and close if it's too much hassle.

[alex]

If I'm following correctly, there's really two behavior inconsistencies that this issue is tackling:

1. TypeError vs. ValueError for not presenting a password when a key is encrypted
2. Error vs. no-error for presenting a password when a key isn't encrypted.

Is that right?

While not stated explicitly, I'm assuming you'd prefer that SSH adopt the PEM behavior?

[hannob]

Yes and Yes.

[reaperhulk]

Error when presenting a password for an unencrypted key is quite simple to implement, but the other is more breaking (as you already noted) since we would be changing the type of the exception for a bad checksum. I'm not sure the value of consistency here outweighs the low grade incompatibility...

[alex]

What does checksum have to do with it?

…

On Sat, Nov 30, 2024 at 5:38 PM Paul Kehrer ***@***.***> wrote: Error when presenting a password for an unencrypted key is quite simple to implement, but the other is more breaking (as you already noted) since we would be changing the type of the exception for a bad checksum. I'm not sure the value of consistency here outweighs the low grade incompatibility... — Reply to this email directly, view it on GitHub <#12070 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBD4YGK4JTAGKPRFTG32DI44ZAVCNFSM6AAAAABSW4FAXOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMBZGQZDEOBUGY> . You are receiving this because you commented.Message ID: ***@***.***>

-- All that is necessary for evil to succeed is for good people to do nothing.

[reaperhulk]

The first check after decryption is if two values within the deserialized object match. Looking at the spec those "checkints" (which we call checksums in our codebase for some reason) exist specifically to determine if a decryption has failed. https://github.com/openssh/openssh-portable/blob/67ace92be0718df7e0f52c0a76684fc2ebae4089/PROTOCOL.key#L54-L57

So it's not as bad as I previously believed -- there should not be other scenarios where this triggers. It is still an exception type change, but more constrained.

[alex]

Sorry, I'm not sure I follow. My impression was that the two chnages to be made were:

1. https://github.com/pyca/cryptography/blob/main/src/cryptography/hazmat/primitives/serialization/ssh.py#L748 raise an exception here it password was provided
2. https://github.com/pyca/cryptography/blob/main/src/cryptography/hazmat/primitives/serialization/ssh.py#L199 change the exception type

[reaperhulk]

I think I misread the original issue and thought the claim was a TypeError was occurring even on wrong password in the asymmetric PEM path (and that the proposal was to align all behavior).