-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Separate functions to generate Signature and Authorization headers #36
Separate functions to generate Signature and Authorization headers #36
Conversation
liamdennehy
commented
Aug 17, 2018
•
edited
Loading
edited
- Different use cases for each (authorisation vs digital signatures)
- Different meanings deserve distinct flows
- A Signature may need to be be added to a message with an existing Authorization header so can't presume we can simply generate both on every invocation
src/Signer.php
Outdated
*/ | ||
public function authorize($message) | ||
{ | ||
$signatureParameters = $this->signatureParameters($message); | ||
$message = $message->withAddedHeader("Authorization", "Signature " . $signatureParameters->string()); | ||
return $message; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm this is a BC break.
What actually is the difference between Signature
and Authorization
anyway? Seems like the same data is on each header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They have the same cryptographic signatures if signing the same data (headers) but serve two different intents, and the distinction is crucial:
Authorization
is a claim that I am authorised to make a statement. For a token request, it's "I am a particular identity, I can prove it with this signature derived from a secret only I know, and would like to perform an action.". It could be a signature, a username/password, APIKey etc, but is generally proof of identity through proof of knowledge/possession. The signature is incidental to the purpose.Signature
is an assertion that information is correct - including the request I am making(request-target)
, headers, signed body etc) has not changed during transmission - something not explicitly asserted in an authorisation claim. The requester typically accompanies this with a separateAuthorization
artefact (e.g. a Bearer token, or indeed maybe a HTTP signature but it is distinct and may be signed differently to convey the different intent).- The
Signature
is especially important for some requests e.g. My Signature asserts I have correctly formatted the body of my POST action to initiate a payment to someone - you can trust I have actually sent this and crucially I cannot deny I sent it at a later time (non-repudiation).
Crucially a specific request may require an Authorization
header with e.g. a Bearer token, which we would either overwrite or add a second instance of using this library. See for example page 36 of this standard - the Authorization
header carries an unrelated token (to authorise the request) alongside a Signature
header (which validates the content of the request and that it was explicitly and intentionally made by the requester with the force of a contract).
Even outside implementation constraints lumping them together doesn't force the requester to think about exactly which claim they are making. "It just works" serves nobody in Information Security, and it's unlikely an endpoint would require both headers under the HTTP Signatures scheme. If so, explicitly request both with ->sign()
and ->authorize()
, for each intent (specifically, Contexts).
Apologies for the long explanation, I'm actually an InfoSec professional not a developer, and getting this right matters to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What actually is the difference between
Signature
andAuthorization
anyway? Seems like the same data is on each header.
@mtibben Please check w3c-ccg/http-signatures#48 and see if it is clear, these are quite distinct so important to make sure it's right and communicated properly for non-specialists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm consulting on an implementation now where the Authorization header uses a (bearer) token retrieved from an OAuth2 service, and Signature header from this protocol. Using this library as-is (generating both) would either overwrite the Authorization header (losing the token) or add a second Authorization header, which most implementations would probably not understand or some may outright reject if they're not expecting it.
This is a required change IMHO, though I realise the potential for BC impact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This distinction is now part of the RFC version 11, clearly spelled out in section 1. Since they serve distinct purposes, and can collide with existing headers, I don't see how we can carry on generating both in all cases.
cfd8543
to
a875c1a
Compare
@@ -28,11 +31,19 @@ public function __construct($message, KeyStoreInterface $keyStore) | |||
/** | |||
* @return bool | |||
*/ | |||
public function isValid() | |||
public function isSigned() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a BC break. Could we keep (and deprecate) the isValid
method instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure what the strategy would be, advise?
Similarly the existing sign()
generates Signature
and Authorization
, so generating only Signature
would be a BC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep isValid
function around, mark it as deprecated and return isSigned && isAuthorized
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can re-introduce isValid()
, but don't know the approach to deprecate. Is this a hint, a warning emitted in runtime (print()
?), documentation only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtibben Any advice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtibben, I need to know how deprecation is achieved to get this finished, any suggestions?
Could you rebase out the merge commit? |
* Include tests to confirm
5243a87
to
c63bbee
Compare
* Only available in >=PHP-5.6, which was just bumped to new minimum version * http://php.net/manual/en/function.hash-equals.php * From above: Apparently important user-supplied string is second parameter.
c63bbee
to
f2ea38d
Compare
/** | ||
* @return string | ||
* | ||
* @throws Exception |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
says throws Exception
but the method doesn't
* @return HmacAlgorithm | ||
* @return Key | ||
* | ||
* @throws Exception |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
says throws Exception
but the method doesn't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the invoked method does (following the chain). Is this hint only for direct throws in this function, or unhandled exceptions in all further called functions within?
Compare src/Context.php with @throws
hints for both the directly thrown and invoked functions' thrown exceptions.:
/**
* @return Key
*
* @throws Exception
* @throws KeyStoreException
*/
private function signingKey()
{
if (isset($this->signingKeyId)) {
return $this->keyStore()->fetch($this->signingKeyId);
} else {
throw new Exception('no implicit or specified signing key');
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtibben Happy to remove these hints, but I want to be consistent so your advice is appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump...
/** | ||
* @return Algorithm | ||
* | ||
* @throws Exception |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
says throws Exception
but the method doesn't
} | ||
|
||
/** | ||
* @return Algorithm | ||
* | ||
* @throws Exception |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here
This PR is now mirrored in a PR for the specification itself (w3c-ccg/http-signatures#48) to make it more clear that these headers serve different purposes - while both Signature and Authorization can be used for authorisation, Signature is distinct in that it can create a digitally signed document out of a HTTP Message. I can even imagine a use case where Signature can sign the Authorization header, so we need to make these distinct. |
2fabd9e
to
21e178f
Compare
I have integrated this feature in my own project along with a number of others, and published this in packagist. Documentation for the entire library is published at Read the Docs: http-signatures-php - incomplete but being expanded regularly. |
- Remove unsupported Symfony/PHP version combinations - Add PHP 7.3 support
5fa5092
to
f34e4dd
Compare
Closing PR due to inactivity. |