Skip to content
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

Signed requests #45979

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Signed requests #45979

wants to merge 2 commits into from

Conversation

ArtificialOwl
Copy link
Member

@ArtificialOwl ArtificialOwl commented Jun 19, 2024

Signed Request

Signing request allows to confirm the identity of the sender.
Signing request does not encrypt nor affect its payload.
Signing request only adds metadata to the headers of the request.

Signature

The concept is to add unique metadata and sign them using a private/public key pair.
The location of the public key used to verify the signature will confirm the origin of the request.

Signature does not affect the data of the request, it only adds headers to it:

 {
     "(request-target)": "post /path",
     "content-length": 380,
     "date": "Mon, 08 Jul 2024 14:16:20 GMT",
     "digest": "SHA-256=U7gNVUQiixe5BRbp4Tg0xCZMTcSWXXUZI2\\/xtHM40S0=",
     "host": "hostname.of.the.recipient",
     "Signature": "keyId=\"https://author.hostname/key\",algorithm=\"ras-sha256\",headers=\"content-length date digest host\",signature=\"DzN12OCS1rsA[...]o0VmxjQooRo6HHabg==\""
 }
  • 'content-length' is the total length of the data/content
  • 'date' is the datetime the request have been initiated
  • 'digest' is a checksum of the data/content
  • 'host' is the hostname of the recipient of the request (remote when signing outgoing request, local on incoming request)
  • 'Signature' contains the signature generated using the private key, and metadata:
    • 'keyId' is a unique id, formatted as an url. hostname is used to retrieve the public key via custom discovery
    • 'algorithm' define the algorithm used to generate signature
    • 'headers' contains a list of element used during the generation of the signature
    • 'signature' is the encrypted string, using local private key, of an array containing elements
      listed in 'headers' and their value. Some elements (content-length date digest host) are mandatory
      to ensure authenticity override protection.

(Those are the minimum required headers, some can be added via options during the process)

ISignatoryManager

Because each protocol have different ways to obtain the public key of a remote instance or entity, some part of the signing/verifying process is managed by a custom provider, one for each protocol.

  • getProviderId should returns a unique string

  • getOptions should returns an array that can contains those entries:

    • 'ttl' (300) is the lifetime (in secondes) of the signature
    • 'ttlSignatory' (86400*3) the cache lifetime on a remote signatory
    • 'extraSignatureHeaders' ([]) (list of extra headers to include to the signature)
    • 'algorithm' ('sha256') is the algorithm used to generate the signature
    • 'dateHeader' ("D, d M Y H:i:s T") the format of the 'date' header
  • getLocalSignatory should return the local signatory, including the full (public+private) key pair.

  • getRemoteSignatory should returns a remote signatory based on the requested data, must at least contains key id and public key

IKeyPairManager

IKeyPairManager contains a group of method to create/manage/store internal public/private key pair, stored as sensitive data using a lazy loaded IAppConfig variable. 2 strings app id and name are used to identify key pairs.

  • generateKeyPair generate and store public/private key pair.
  • hasKeyPair return if key pair is known in database.
  • getKeyPair return key pair from database based on app id and name.
  • deleteKeyPair delete key pair from database.

ISignatureManager

ISignatureManager is a service integrated to core that provide tools to set/get authenticity of/from outgoing/incoming requests.

  • getIncomingSignedRequest extract data from the incoming request and compare headers to confirm authenticity of remote instance
  • getOutgoingSignedRequest prep signature to sign an outgoing request.
  • signOutgoingRequestIClientPayload is the one method to call to fully process of signing and fulfilling the payload for an outgoing request using IClient
  • searchSignatory get a remote signatory from the database

lib/private/OCM/OCMSignatoryManager.php Fixed Show fixed Hide fixed
lib/private/OCM/OCMSignatoryManager.php Fixed Show fixed Hide fixed
lib/private/OCM/OCMSignatoryManager.php Fixed Show fixed Hide fixed
lib/private/OCM/OCMSignatoryManager.php Fixed Show fixed Hide fixed
lib/private/OCM/OCMSignatoryManager.php Fixed Show fixed Hide fixed

[$k, $v] = explode('=', $entry, 2);
preg_match('/"([^"]+)"/', $v, $varr);
if ($varr[0] !== null) {

Check failure

Code scanning / Psalm

RedundantCondition

string can never contain null
lib/private/OCM/OCMSignatoryManager.php Fixed Show fixed Hide fixed
lib/private/OCM/OCMSignatoryManager.php Fixed Show fixed Hide fixed

$parsed = parse_url($uri);
$signedRequest->setHost($parsed['host'])
->setAlgorithm($options['algorithm'] ?? 'sha256')

Check failure

Code scanning / Psalm

UndefinedVariable

Cannot find referenced variable $options
apps/files_sharing/lib/External/Cache.php Fixed Show fixed Hide fixed
// remote does not support signed request.
// currently we still accept unsigned request until lazy appconfig
// core.enforce_signed_ocm_request is set to true (default: false)
if ($this->appConfig->getValueBool('enforce_signed_ocm_request', false, lazy: true)) {

Check failure

Code scanning / Psalm

InvalidArgument

Argument 2 of OCP\IAppConfig::getValueBool cannot be false, string value expected
if ($signatory === null) {
throw new SignatoryNotFoundException('empty result from getRemoteSignatory');
}
if ($signatory->getKeyId() !== $signedRequest->getKeyId()) {

Check failure

Code scanning / Psalm

UndefinedInterfaceMethod

Method OCP\Security\Signature\Model\IIncomingSignedRequest::getKeyId does not exist
if (!str_contains($entry, '@')) {
throw new IncomingRequestException('entry does not contains @');
}
[, $instance] = explode('@', $entry, 2);

Check notice

Code scanning / Psalm

PossiblyUndefinedArrayOffset Note

Possibly undefined array key
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
* @inheritDoc
*
* @param string $publicKey
* @return IKeyPair

Check failure

Code scanning / Psalm

MismatchingDocblockReturnType Error

Docblock has incorrect return type 'OCP\Security\PublicPrivateKeyPairs\Model\IKeyPair', should be 'OC\Security\PublicPrivateKeyPairs\Model\KeyPair'
* @inheritDoc
*
* @param string $privateKey
* @return IKeyPair

Check failure

Code scanning / Psalm

MismatchingDocblockReturnType Error

Docblock has incorrect return type 'OCP\Security\PublicPrivateKeyPairs\Model\IKeyPair', should be 'OC\Security\PublicPrivateKeyPairs\Model\KeyPair'
* @inheritDoc
*
* @param array $options
* @return IKeyPair

Check failure

Code scanning / Psalm

MismatchingDocblockReturnType Error

Docblock has incorrect return type 'OCP\Security\PublicPrivateKeyPairs\Model\IKeyPair', should be 'OC\Security\PublicPrivateKeyPairs\Model\KeyPair'
* @inheritDoc
*
* @param string $host
* @return IOutgoingSignedRequest

Check failure

Code scanning / Psalm

MismatchingDocblockReturnType Error

Docblock has incorrect return type 'OCP\Security\Signature\Model\IOutgoingSignedRequest', should be 'OC\Security\Signature\Model\OutgoingSignedRequest'
* @param string $key
* @param string|int|float|bool|array $value
*
* @return IOutgoingSignedRequest

Check failure

Code scanning / Psalm

MismatchingDocblockReturnType Error

Docblock has incorrect return type 'OCP\Security\Signature\Model\IOutgoingSignedRequest', should be 'OC\Security\Signature\Model\OutgoingSignedRequest'
*
* @param string $estimated
*
* @return IOutgoingSignedRequest

Check failure

Code scanning / Psalm

MismatchingDocblockReturnType Error

Docblock has incorrect return type 'OCP\Security\Signature\Model\IOutgoingSignedRequest', should be 'OC\Security\Signature\Model\OutgoingSignedRequest'
*
* @param string $algorithm
*
* @return IOutgoingSignedRequest

Check failure

Code scanning / Psalm

MismatchingDocblockReturnType Error

Docblock has incorrect return type 'OCP\Security\Signature\Model\IOutgoingSignedRequest', should be 'OC\Security\Signature\Model\OutgoingSignedRequest'
* @inheritDoc
*
* @param array $signatureHeader
* @return ISignedRequest

Check failure

Code scanning / Psalm

MismatchingDocblockReturnType Error

Docblock has incorrect return type 'OCP\Security\Signature\Model\ISignedRequest', should be 'OC\Security\Signature\Model\SignedRequest'
/**
* @inheritDoc
*
* @return ISignatory

Check failure

Code scanning / Psalm

InvalidNullableReturnType Error

The declared return type 'OCP\Security\Signature\Model\ISignatory' for OC\Security\Signature\Model\SignedRequest::getSignatory is not nullable, but 'OCP\Security\Signature\Model\ISignatory|null' contains null
* @since 30.0.0
*/
public function getSignatory(): ISignatory {
return $this->signatory;

Check failure

Code scanning / Psalm

NullableReturnStatement Error

The declared return type 'OCP\Security\Signature\Model\ISignatory' for OC\Security\Signature\Model\SignedRequest::getSignatory is not nullable, but the function returns 'OCP\Security\Signature\Model\ISignatory|null'
@ArtificialOwl ArtificialOwl added the 3. to review Waiting for reviews label Jul 10, 2024
@ArtificialOwl ArtificialOwl added this to the Nextcloud 30 milestone Jul 10, 2024
@ArtificialOwl ArtificialOwl marked this pull request as ready for review July 10, 2024 16:24
to.json Outdated Show resolved Hide resolved
// if request is signed and well signed, no exception are thrown
// if request is not signed and host is known for not supporting signed request, no exception are thrown
$signedRequest = $this->getSignedRequest();
$this->confirmShareOrigin($signedRequest, $notification['sharedSecret'] ?? '');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sharedSecret inside the notification is already used by some OCM messages. So this would break it. Can you take another key? Or maybe you prefix with '$#$' or something alike?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would not break it as I send the exception for the exact same reason of a missing 'sharedSecret' entry in the notifications request. The only thing is that I do this check earlier than others but I can add a prefix if you want (while I dont feel like necessary myself)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would not break it as I send the exception for the exact same reason of a missing 'sharedSecret' entry in the notifications request.

Nextcloud Talk is sending a data field 'sharedSecret' and you either overwrite that and it breaks Talk Federation with older servers or you need to use a different key

lib/private/Federation/CloudFederationProviderManager.php Outdated Show resolved Hide resolved
lib/private/Security/Signature/Model/SignedRequest.php Outdated Show resolved Hide resolved
lib/private/Security/Signature/SignatureManager.php Outdated Show resolved Hide resolved
core/Migrations/Version30000Date20240101084401.php Outdated Show resolved Hide resolved
}
}

private function insertSignatory(ISignatory $signatory): void {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By now our Entity+QBMapper pattern is widely adapted. Any reason why you didn't go for it and instead went back to doing all this manually again?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no real reason other that personal preference

lib/private/Security/Signature/SignatureManager.php Outdated Show resolved Hide resolved
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants