GNOME Bugzilla – Bug 699161
Add support for symmetric encryption to CryptoService.EncryptText
Last modified: 2016-08-02 15:17:10 UTC
Created attachment 242734 [details] [review] Add support for symmetric encryption In order to implement symmetric encryption in Seahorse (#325803), a first step is to add support for symmetric encryption in CryptoService.EncryptText. This is done by the attached patches. The interface is the same as the one provided by gpgme_op_encrypt_sign_start(): if the list of recipients is empty, then symmetric encryption is performed. Users will be prompted for the passphrase through the default callback — seahorse-gpgme-source.c:passphrase_get() — like currently done for symmetric decryption.
Created attachment 242736 [details] [review] Fix white spaces in crypto_encrypt_generic() Whitespace only changes, to be applied before the implementation patch.
Review of attachment 242736 [details] [review]: I know it might be annoying, but doing white space rewrites loses history. Generally I try to clean up whitespace as I refactor a function, but not reformatting just because.
Review of attachment 242734 [details] [review]: Looks like a good patch, with the tweak I mention below. For what it's worth, I'd like to retire libcryptui at some point (or at least my involvement in it). Just a heads up. Eventually a new maintainer will need to be found, or it'll be deprecated (in favor of using GPGME directly, and using Gcr for prompting). ::: daemon/seahorse-service-crypto.c @@ +275,3 @@ * crypto: the crypto service (#SeahorseServiceCrypto) * recipients: A list of recipients (keyids "openpgp:B8098FB063E2C811") +* If the list is empty, symmetric encryption will be used. Do you think it makes sense to use a flag instead, and make this more explicit? Otherwise it might be a source of bugs.
About whitespaces: I separated reformating from the actual code changes to make the later easier to read. I could merge these two patches if you'd like or drop the first (but the indentation would even be less consistent than it is now). I think the changes to implement #325803 should be small enough to be included in libcryptui despite a future retirement. It might also be a good opportunity to experiment user interfaces that can also support symmetric encryption. I could be interested in porting this work to the foreseen infrastructure later on. I had 3 reasons to go with the "empty recipients" = "symmetric" way: 1. It doesn't change the ABI. I don't know how many users of the interface exists and how much work it would be to switch them to a new API. 2. Symmetric encryption makes the list of recipients useless. This means that, IMHO, the only proper behavior with a flag would be to error out if there are recipients in the list and symmetric encryption is selected. In a language like Haskell, Go or Rust, I would suggest using a datatype like `SymmetricEncryption | PublicKeyEncryption([PubKey])` but this is C… I just went without the extra bit of information, although I agree that it might be a source of bugs. 3. This is the same interface as gpgme_op_encrypt_sign_start(). If you are thinking of deprecating libcryptui in favor of direct use of gpgme, the last one might actually ease future porting. If you feel an extra flag is better, I would welcome guidance on the best way to handle the resulting ABI change.
(In reply to comment #4) > About whitespaces: I separated reformating from the actual code changes to make > the later easier to read. I could merge these two patches if you'd like or > drop the first (but the indentation would even be less consistent than it is > now). If you're going to rewrite nearly all of the function, then sure, you could reformat as part of the same commit. > I think the changes to implement #325803 should be small enough to be included > in libcryptui despite a future retirement. It might also be a good opportunity > to experiment user interfaces that can also support symmetric encryption. > I could be interested in porting this work to the foreseen infrastructure > later on. > > I had 3 reasons to go with the "empty recipients" = "symmetric" way: > > 1. It doesn't change the ABI. I don't know how many users of the interface > exists and how much work it would be to switch them to a new API. The flags field is already there but unused. Thus this wouldn't be an ABI break. > 2. Symmetric encryption makes the list of recipients useless. This means > that, IMHO, the only proper behavior with a flag would be to error > out if there are recipients in the list and symmetric encryption is > selected. Agreed. > 3. This is the same interface as gpgme_op_encrypt_sign_start(). Yes, but it's been that way from the beginning and documetned as such. There's been questions on IRC about what the behavior of EncryptText should be when no recipients are selected (should it just encrypt to the default key, ie: self?).
Created attachment 243054 [details] [review] Add support for symmetric encryption (v2)
(In reply to comment #5) > (In reply to comment #4) > > About whitespaces: I separated reformating from the actual code changes to make > > the later easier to read. I could merge these two patches if you'd like or > > drop the first (but the indentation would even be less consistent than it is > > now). > > If you're going to rewrite nearly all of the function, then sure, you could > reformat as part of the same commit. I'll proceed this way in the future. > The flags field is already there but unused. Thus this wouldn't be an ABI > break. I had overlooked that there already was a `flags` argument. Thanks for pointing that out! Please have a look at the new patch which uses a new flag to tell the service that it should perform symmetric encryption. The function will error out if any recipient is specified at the same time.
Committed patch with a few tweaks.