Change sq key delete to refuse to work with weakly bound subkeys.
- `sq key delete` deletes all secret key material associated with a
certificate. Of course, we don't want to delete secret key
material that we are not confident belongs to the certificate.
- Imagine Alice creates a new certificate. Mallory see this, and
anticipates that she is going to delete the old certificate. He
attaches her new encryption-capable subkey to the old certificate
using some weak cryptography, publishes it, and then Alice gets
the update to her old certificate via parcimonie. When she
deletes the secret key material associated with the old
certificate, she would also delete her new secret key material.
Ouch! Admittedly, this attack is a bit contrived.
- Alternatively, we could skip subkeys whose bindings rely on
weak cryptography. This behavior would probably surprise most
users. It could have serious consequences as well, since the
user thought they deleted the secret key material, but didn't.
- Instead, we are conservative: if a subkey's binding signature
relies on weak cryptography AND we have secret key material for
it, we abort, and suggest using `sq key subkey delete` instead.
- See #375 and #457.
2024-11-21 16:02:06 +03:00
use std ::collections ::HashSet ;
2024-07-05 00:06:44 +03:00
use sequoia_openpgp as openpgp ;
Change sq key delete to refuse to work with weakly bound subkeys.
- `sq key delete` deletes all secret key material associated with a
certificate. Of course, we don't want to delete secret key
material that we are not confident belongs to the certificate.
- Imagine Alice creates a new certificate. Mallory see this, and
anticipates that she is going to delete the old certificate. He
attaches her new encryption-capable subkey to the old certificate
using some weak cryptography, publishes it, and then Alice gets
the update to her old certificate via parcimonie. When she
deletes the secret key material associated with the old
certificate, she would also delete her new secret key material.
Ouch! Admittedly, this attack is a bit contrived.
- Alternatively, we could skip subkeys whose bindings rely on
weak cryptography. This behavior would probably surprise most
users. It could have serious consequences as well, since the
user thought they deleted the secret key material, but didn't.
- Instead, we are conservative: if a subkey's binding signature
relies on weak cryptography AND we have secret key material for
it, we abort, and suggest using `sq key subkey delete` instead.
- See #375 and #457.
2024-11-21 16:02:06 +03:00
use openpgp ::Cert ;
use openpgp ::Fingerprint ;
2024-11-22 13:56:55 +03:00
use openpgp ::KeyHandle ;
2024-07-05 00:06:44 +03:00
use openpgp ::Result ;
Change sq key delete to refuse to work with weakly bound subkeys.
- `sq key delete` deletes all secret key material associated with a
certificate. Of course, we don't want to delete secret key
material that we are not confident belongs to the certificate.
- Imagine Alice creates a new certificate. Mallory see this, and
anticipates that she is going to delete the old certificate. He
attaches her new encryption-capable subkey to the old certificate
using some weak cryptography, publishes it, and then Alice gets
the update to her old certificate via parcimonie. When she
deletes the secret key material associated with the old
certificate, she would also delete her new secret key material.
Ouch! Admittedly, this attack is a bit contrived.
- Alternatively, we could skip subkeys whose bindings rely on
weak cryptography. This behavior would probably surprise most
users. It could have serious consequences as well, since the
user thought they deleted the secret key material, but didn't.
- Instead, we are conservative: if a subkey's binding signature
relies on weak cryptography AND we have secret key material for
it, we abort, and suggest using `sq key subkey delete` instead.
- See #375 and #457.
2024-11-21 16:02:06 +03:00
use openpgp ::cert ::amalgamation ::ValidAmalgamation ;
use openpgp ::parse ::Parse ;
use openpgp ::types ::RevocationStatus ;
2024-07-05 00:06:44 +03:00
2024-08-15 14:38:43 +03:00
use super ::common ::Sq ;
Change sq key delete to refuse to work with weakly bound subkeys.
- `sq key delete` deletes all secret key material associated with a
certificate. Of course, we don't want to delete secret key
material that we are not confident belongs to the certificate.
- Imagine Alice creates a new certificate. Mallory see this, and
anticipates that she is going to delete the old certificate. He
attaches her new encryption-capable subkey to the old certificate
using some weak cryptography, publishes it, and then Alice gets
the update to her old certificate via parcimonie. When she
deletes the secret key material associated with the old
certificate, she would also delete her new secret key material.
Ouch! Admittedly, this attack is a bit contrived.
- Alternatively, we could skip subkeys whose bindings rely on
weak cryptography. This behavior would probably surprise most
users. It could have serious consequences as well, since the
user thought they deleted the secret key material, but didn't.
- Instead, we are conservative: if a subkey's binding signature
relies on weak cryptography AND we have secret key material for
it, we abort, and suggest using `sq key subkey delete` instead.
- See #375 and #457.
2024-11-21 16:02:06 +03:00
use super ::common ::STANDARD_POLICY ;
2024-07-05 00:06:44 +03:00
#[ test ]
fn sq_key_delete ( ) -> Result < ( ) > {
let sq = Sq ::new ( ) ;
let ( cert , cert_file , _rev_file ) = sq . key_generate ( & [ ] , & [ " alice " ] ) ;
assert! ( cert . is_tsk ( ) ) ;
// Delete all the secret key material from a certificate stored in
// a file. Make sure the result contains no secret key material.
2024-10-22 19:13:31 +03:00
let updated = sq . key_delete ( & cert_file ,
std ::path ::PathBuf ::from ( " - " ) . as_path ( ) ) ;
2024-07-05 00:06:44 +03:00
assert! ( ! updated . is_tsk ( ) ) ;
// Do the same for a certificate whose secret key material is
// managed by the keystore.
sq . key_import ( cert_file ) ;
let cert = sq . key_export ( cert . key_handle ( ) ) ;
assert! ( cert . is_tsk ( ) ) ;
let updated = sq . key_delete ( cert . key_handle ( ) , None ) ;
assert! ( ! updated . is_tsk ( ) ) ;
// If we really stripped the secret key material, then `sq key
// export` will fail.
assert! ( sq . key_export_maybe ( cert . key_handle ( ) ) . is_err ( ) ) ;
Ok ( ( ) )
}
Change sq key delete to refuse to work with weakly bound subkeys.
- `sq key delete` deletes all secret key material associated with a
certificate. Of course, we don't want to delete secret key
material that we are not confident belongs to the certificate.
- Imagine Alice creates a new certificate. Mallory see this, and
anticipates that she is going to delete the old certificate. He
attaches her new encryption-capable subkey to the old certificate
using some weak cryptography, publishes it, and then Alice gets
the update to her old certificate via parcimonie. When she
deletes the secret key material associated with the old
certificate, she would also delete her new secret key material.
Ouch! Admittedly, this attack is a bit contrived.
- Alternatively, we could skip subkeys whose bindings rely on
weak cryptography. This behavior would probably surprise most
users. It could have serious consequences as well, since the
user thought they deleted the secret key material, but didn't.
- Instead, we are conservative: if a subkey's binding signature
relies on weak cryptography AND we have secret key material for
it, we abort, and suggest using `sq key subkey delete` instead.
- See #375 and #457.
2024-11-21 16:02:06 +03:00
#[ test ]
fn unbound_subkey ( ) {
// Make sure we don't delete secret key material if there is an
// unbound subkey.
let sq = Sq ::new ( ) ;
let cert_path = sq . test_data ( )
. join ( " keys " )
. join ( " unbound-subkey.pgp " ) ;
let cert = Cert ::from_file ( & cert_path ) . expect ( " can read " ) ;
let vc = cert . with_policy ( STANDARD_POLICY , sq . now ( ) )
. expect ( " valid cert " ) ;
// One subkey should be considered invalid.
let bound : HashSet < Fingerprint >
= HashSet ::from_iter ( vc . keys ( ) . map ( | ka | ka . fingerprint ( ) ) ) ;
let all : HashSet < Fingerprint >
= HashSet ::from_iter ( cert . keys ( ) . map ( | ka | ka . fingerprint ( ) ) ) ;
assert! ( bound . len ( ) < all . len ( ) ) ;
let result = sq . key_delete ( & cert_path , None ) ;
for ka in result . keys ( ) {
if bound . contains ( & ka . fingerprint ( ) ) {
assert! ( ! ka . has_secret ( ) ) ;
} else {
assert! ( ka . has_secret ( ) ) ;
}
}
}
#[ test ]
fn soft_revoked_subkey ( ) {
// Make sure we can delete a soft revoked subkey.
let sq = Sq ::new ( ) ;
let cert_path = sq . test_data ( )
. join ( " keys " )
. join ( " soft-revoked-subkey.pgp " ) ;
let cert = Cert ::from_file ( & cert_path ) . expect ( " can read " ) ;
let vc = cert . with_policy ( STANDARD_POLICY , sq . now ( ) )
. expect ( " valid cert " ) ;
// Make sure the revoked key is there and is really revoked.
let mut revoked = None ;
for k in vc . keys ( ) . subkeys ( ) {
if let RevocationStatus ::Revoked ( _ ) = k . revocation_status ( ) {
assert! ( revoked . is_none ( ) ,
" Only expected a single revoked subkey " ) ;
revoked = Some ( k . key_handle ( ) ) ;
}
}
if revoked . is_none ( ) {
panic! ( " Expected a revoked subkey, but didn't fine one " ) ;
}
let updated = sq . key_delete ( cert_path , None ) ;
assert! ( ! updated . is_tsk ( ) ) ;
}
#[ test ]
fn hard_revoked_subkey ( ) {
// Make sure we can delete a hard revoked subkey.
let sq = Sq ::new ( ) ;
let cert_path = sq . test_data ( )
. join ( " keys " )
. join ( " hard-revoked-subkey.pgp " ) ;
let cert = Cert ::from_file ( & cert_path ) . expect ( " can read " ) ;
let vc = cert . with_policy ( STANDARD_POLICY , sq . now ( ) )
. expect ( " valid cert " ) ;
// Make sure the revoked key is there and is really revoked.
let mut revoked = None ;
for k in vc . keys ( ) . subkeys ( ) {
if let RevocationStatus ::Revoked ( _ ) = k . revocation_status ( ) {
assert! ( revoked . is_none ( ) ,
" Only expected a single revoked subkey " ) ;
revoked = Some ( k . key_handle ( ) ) ;
}
}
if revoked . is_none ( ) {
panic! ( " Expected a revoked subkey, but didn't fine one " ) ;
}
let updated = sq . key_delete ( cert_path , None ) ;
assert! ( ! updated . is_tsk ( ) ) ;
}
#[ test ]
fn sha1_subkey ( ) {
// Make sure we can't delete secret key material if there is a
// subkey that is bound using SHA-1.
let sq = Sq ::new ( ) ;
let cert_path = sq . test_data ( )
. join ( " keys " )
. join ( " sha1-subkey-priv.pgp " ) ;
let cert = Cert ::from_file ( & cert_path ) . expect ( " can read " ) ;
let vc = cert . with_policy ( STANDARD_POLICY , sq . now ( ) )
. expect ( " valid cert " ) ;
// Make sure the subkey key is there and really uses SHA-1.
let valid_subkeys : Vec < _ > = vc . keys ( ) . subkeys ( )
. map ( | ka | ka . fingerprint ( ) )
. collect ( ) ;
let all_subkeys : Vec < _ > = cert . keys ( ) . subkeys ( )
. map ( | ka | ka . fingerprint ( ) )
. collect ( ) ;
assert_eq! ( valid_subkeys . len ( ) , 0 ) ;
assert_eq! ( all_subkeys . len ( ) , 1 ) ;
assert! ( sq . try_key_delete ( cert_path , None ) . is_err ( ) ) ;
}
2024-11-22 13:56:55 +03:00
#[ test ]
fn sha1_subkey_without_secret_key_material ( ) {
// Make sure we can delete secret key material in the presence of
// a subkey that is bound using SHA-1, but for which there is no
// secret key material.
let sq = Sq ::new ( ) ;
let cert_path = sq . test_data ( )
. join ( " keys " )
. join ( " sha1-subkey-priv.pgp " ) ;
let cert = Cert ::from_file ( & cert_path ) . expect ( " can read " ) ;
let vc = cert . with_policy ( STANDARD_POLICY , sq . now ( ) )
. expect ( " valid cert " ) ;
// Make sure the subkey key is there and really uses SHA-1.
eprintln! ( " Valid keys: " ) ;
let valid_keys : Vec < _ > = vc . keys ( )
. map ( | ka | {
let fpr = ka . fingerprint ( ) ;
eprintln! ( " - {} " , fpr ) ;
fpr
} )
. collect ( ) ;
eprintln! ( " All keys: " ) ;
let all_keys : Vec < _ > = cert . keys ( )
. map ( | ka | {
let fpr = ka . fingerprint ( ) ;
eprintln! ( " - {} " , fpr ) ;
fpr
} )
. collect ( ) ;
assert! ( valid_keys . len ( ) < all_keys . len ( ) ) ;
let mut update = cert_path ;
for fpr in all_keys . iter ( ) {
if ! valid_keys . contains ( fpr ) {
let update2 = sq . scratch_file (
Some ( & format! ( " delete- {} " , fpr ) [ .. ] ) ) ;
sq . key_subkey_delete (
update , & [ KeyHandle ::from ( fpr ) ] , update2 . as_path ( ) ) ;
update = update2 ;
}
}
let cert = Cert ::from_file ( & update ) . expect ( " can read " ) ;
for ka in cert . keys ( ) {
if valid_keys . contains ( & ka . fingerprint ( ) ) {
assert! ( ka . has_secret ( ) ) ;
} else {
assert! ( ! ka . has_secret ( ) ,
" {} still has secret key material " , ka . fingerprint ( ) ) ;
}
}
let result = sq . key_delete ( update , None ) ;
assert! ( ! result . is_tsk ( ) ) ;
}
2024-11-22 15:57:04 +03:00
#[ test ]
fn ambiguous ( ) {
// If a key is associated with multiple certificates, then sq key
// delete should refuse to delete the secret key material.
let sq = Sq ::new ( ) ;
let ( alice1 , alice1_path , _alice1_rev )
= sq . key_generate ( & [ ] , & [ " alice1 " ] ) ;
sq . key_import ( & alice1_path ) ;
let common_subkey = alice1 . keys ( ) . subkeys ( ) . take ( 1 )
. map ( | ka | ka . key_handle ( ) )
. collect ::< Vec < _ > > ( ) ;
let ( alice2 , alice2_path , _alice2_rev )
= sq . key_generate ( & [ ] , & [ " alice2 " ] ) ;
sq . key_import ( & alice2_path ) ;
let alice2_update = sq . scratch_file ( " alice2-updated " ) ;
sq . key_subkey_bind ( & [ ] , vec! [ & alice2_path ] ,
alice2 . fingerprint ( ) ,
common_subkey . clone ( ) ,
& alice2_update ) ;
sq . key_import ( alice2_update ) ;
assert! ( sq . try_key_delete ( alice2 . fingerprint ( ) , None ) . is_err ( ) ) ;
// We should be able to delete it using sq key subkey delete.
sq . key_subkey_delete ( alice1 . fingerprint ( ) ,
& common_subkey ,
None ) ;
sq . key_subkey_delete ( alice2 . fingerprint ( ) ,
& common_subkey ,
None ) ;
// And now we should be able to delete the secret key material
// associated with the certificate.
sq . key_delete ( alice2 . fingerprint ( ) , None ) ;
}