Hi list,
A couple of bug reports have highlighted the fact that our
openssl_encrypt and openssl_decrupt functions have no way of getting
or setting tags required for authenticated cipher modes (i.e. GCM,
CCM, OCB (not sure if this is available in OpenSSL)).https://bugs.php.net/bug.php?id=68962
https://bugs.php.net/bug.php?id=67304Further to this, we have no way of setting any associated data.
I think we absolutely must provide a method for users to be able to
use authenticated encryption, and would like some opinions on how much
flexibility we give users, and the best method for exposing this
functionality.At the very basic end of the spectrum, we could have openssl_get_tag
and openssl_set_tag, or add an extra parameter to the end of
openssl_encrypt and openssl_decrypt (pass by ref for encrypt, like
preg $matches) this would cover the majority of use cases.However I absolutely think that the associated data also needs to be
supported, and possibly the ability to change the tag length.At this point we're starting to get into the territory where an
$options array is needed, or we add a lot of parameters to the end of
functions. I don't really think it's good to add up to 3 more params
to these functions.What do you guys and girls think is the best way of tackling this?
How about ...
Old API:
string openssl_decrypt ( string $data , string $method , string $password
[, int $options = 0 [, string $iv = "" ]] )
string openssl_encrypt ( string $data , string $method , string $password
[, int $options = 0 [, string $iv = "" ]] )
New:
mixed openssl_decrypt ( string $data , string $method , string $password [,
mixed $options = 0 [, string $iv = "" ]] )
string openssl_encrypt ( string $data , string $method , string $password
[, mixed $options = 0 [, string $iv = "" ]] )
The main changes are:
- the $options parameter becomes mixed (either long or array) in both
functions - a long $options parameter triggers
E_DEPRECATEDin php7 (expects array) - the presence of an $iv triggers
E_DEPRECATEDin php7 (scheduled for
removal) -
openssl_decrypt()now returns mixed ... if $options['get_tag'] == true
then return [$decryptedString, $tag], otherwise return $decrypted string as
before to preserve BC. - the encrypt function could use $options['set_tag'] to define that (or
any other secondary information needed for the operation).
This has zero BC implications, emits deprecation warnings for the old way
of doing it and finally provides a schedule for eventual removal of the
excessively verbose API in PHP8.
What I would prefer NOT to see is piling on more optional parameters to
these already too-long function signatures. Also, I don't really like the
idea of adding "state" to this operation with new
openssl_set_tag/openssl_get_tag functions.
Thoughts?
Hey,
openssl_decrypt()now returns mixed ... if $options['get_tag'] == true
then return [$decryptedString, $tag], otherwise return $decrypted string as
before to preserve BC.- the encrypt function could use $options['set_tag'] to define that (or
any other secondary information needed for the operation).
I think that you confused it a bit :). The encryption results in cipher
text and a tag. The decryption then validates the tag so you pass the tag
as a parameter.
Except that it's almost the same what I thought. I'm just not sure that
mixed return value is a good idea. It seems a bit better having tag as a
reference to me. But it's just a small detail that could be added to the
RFC as a choice :)
Cheers
Jakub
Hey,
openssl_decrypt()now returns mixed ... if $options['get_tag'] == true
then return [$decryptedString, $tag], otherwise return $decrypted string
as
before to preserve BC.- the encrypt function could use $options['set_tag'] to define that (or
any other secondary information needed for the operation).I think that you confused it a bit :). The encryption results in cipher
text and a tag. The decryption then validates the tag so you pass the tag
as a parameter.
Okay, you see what I'm getting at :)
Except that it's almost the same what I thought. I'm just not sure that
mixed return value is a good idea. It seems a bit better having tag as a
reference to me. But it's just a small detail that could be added to the
RFC as a choice :)
My personal preference is to minimize the API and not add a sixth (!)
parameter (by-ref at that) to the method signature. I see a mixed return
with a simplified API as the lesser of the two evils, but like you say
that's just bikeshedding and can be part of a brief RFC on the subject.
I should have stated my intent more clearly in the original mail. I
would be targeting 5.5 and above for a core change, and would provide
a an extension to back-fill 5.3 and 5.4. I think people should be able
to use authenticated modes of operation now, not when PHP 7 is
released and / or when it lands in a stable distro.
How about ...
Old API:
string openssl_decrypt ( string $data , string $method , string $password
[, int $options = 0 [, string $iv = "" ]] )
string openssl_encrypt ( string $data , string $method , string $password
[, int $options = 0 [, string $iv = "" ]] )New:
mixed openssl_decrypt ( string $data , string $method , string $password [,
mixed $options = 0 [, string $iv = "" ]] )
string openssl_encrypt ( string $data , string $method , string $password
[, mixed $options = 0 [, string $iv = "" ]] )
Really not sure. On one hand it keeps the parameter count down and
opens the door for other options to be passed in future, on the other
the mixed return type bothers me a little.
The main changes are:
- the $options parameter becomes mixed (either long or array) in both
functions- a long $options parameter triggers
E_DEPRECATEDin php7 (expects array)- the presence of an $iv triggers
E_DEPRECATEDin php7 (scheduled for
removal)
Is the intent to have the IV moved to the options array?
$options = [
'flags' => OPENSSL_RAW_DATA,
'tag' => ...,
'taglen' => 16,
'iv' => ...,
'associated_data' => ...
];
openssl_decrypt()now returns mixed ... if $options['get_tag'] == true
then return [$decryptedString, $tag], otherwise return $decrypted string as
before to preserve BC.- the encrypt function could use $options['set_tag'] to define that (or
any other secondary information needed for the operation).
The result of a mode like GCM is useless without the tag, it should
automatically be provided when you choose a mode that results in a
ciphertext and a tag, and likewise the mode should fail if you try and
decrypt without providing the tag.
What I would prefer NOT to see is piling on more optional parameters to
these already too-long function signatures. Also, I don't really like the
idea of adding "state" to this operation with new
openssl_set_tag/openssl_get_tag functions.
I agree about set/get. My least favourite option.
The extra params aren't really that bad.
string openssl_encrypt ( string $data , string $method , string
$password [, int $options = 0 [, string $iv = "" [, string &$tag [,
string $associated_data ]]]] )
By the time you get to $associated_data, everything preceding it is a
required parameter. Yes it's verbose, but it still doesn't break BC,
and the options and return types remain the same (although this
forgoes the option to specify the tag length - another param would
just be silly ;)).
The extra params aren't really that bad.
Okay, I'd like to reset the conversation a bit here. It's clear that the
current API does not fit the problem domain very well. Tacking on more
parameters only creates a bigger mess. Six parameters to a stateless
function call is a completely incoherent API. It's unusable without
consulting the manual. I think we need a completely different approach. Let
me propose something else that won't break BC in any release and results in
an API that's actually sane:
class CryptoContext {
private $mode;
private $password;
function __construct($mode, $password) {
$this->mode = $mode;
$this->password = $password;
}
function encrypt($data) {
// ...
return $encryptedData;
}
function decrypt($data) {
// ...
return $decryptedData;
}
function setOption($option, $value) {
// ...
}
// more methods here to do anything you need
}
Thoughts on a stateful object API here? Personally I find this much more
coherent than anything that's been proposed so far and it could be
implemented without affecting existing functionality.
The extra params aren't really that bad.
Okay, I'd like to reset the conversation a bit here. It's clear that the
current API does not fit the problem domain very well. Tacking on more
parameters only creates a bigger mess. Six parameters to a stateless
function call is a completely incoherent API. It's unusable without
consulting the manual. I think we need a completely different approach. Let
me propose something else that won't break BC in any release and results in
an API that's actually sane:class CryptoContext {
private $mode;
private $password;
function __construct($mode, $password) {
$this->mode = $mode;
$this->password = $password;
}
function encrypt($data) {
// ...
return $encryptedData;
}
function decrypt($data) {
// ...
return $decryptedData;
}
function setOption($option, $value) {
// ...
}
// more methods here to do anything you need
}Thoughts on a stateful object API here? Personally I find this much more
coherent than anything that's been proposed so far and it could be
implemented without affecting existing functionality.
I'd really love to move to a context based crypto API.
I'd go one further and say I'd really love to see an extensible crypto
API, where extensions could provide ciphers or modes not available in
core.
That works for me going forward, but I'd still like a solution that
can go into 5.x
The extra params aren't really that bad.
Okay, I'd like to reset the conversation a bit here. It's clear that the
current API does not fit the problem domain very well. Tacking on more
parameters only creates a bigger mess. Six parameters to a stateless
function call is a completely incoherent API. It's unusable without
consulting the manual. I think we need a completely different approach. Let
me propose something else that won't break BC in any release and results in
an API that's actually sane:class CryptoContext {
private $mode;
private $password;
function __construct($mode, $password) {
$this->mode = $mode;
$this->password = $password;
}
function encrypt($data) {
// ...
return $encryptedData;
}
function decrypt($data) {
// ...
return $decryptedData;
}
function setOption($option, $value) {
// ...
}
// more methods here to do anything you need
}Thoughts on a stateful object API here? Personally I find this much more
coherent than anything that's been proposed so far and it could be
implemented without affecting existing functionality.
This is sort of what I was trying to do in crypto ext. I ended up with
something like this:
https://github.com/bukka/php-crypto#php-definition-for-the-classes which is
a bit simillar to your proposal and it already works as a bonus :). I'm not
sure if it makes sense to duplicate an effort and do the same thing (and
also call it Crypto which would be quite confusing...) in openssl ext which
is just functional extension. As I said the crypto extension is still in
pecl dev stability so I'm open any changes in it.
If we want to add a cipher context API to openssl ext, then I think that it
would be much better to use a functional API as openssl_pkey_* to keep ti
consistent.
Cheers
Jakub