Signed requests #45979

ArtificialOwl · 2024-06-19T13:49:37Z

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/ocm#signature\",algorithm=\"sha256\",headers=\"request-target) 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 can 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

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

lib/private/Security/Signature/Model/OutgoingSignedRequest.php

lib/private/Security/Signature/Model/SignedRequest.php

lib/private/Security/Signature/SignatureManager.php

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


lib/private/OCM/OCMSignatoryManager.php

lib/private/Security/Signature/Model/SignedRequest.php

lib/private/Security/Signature/SignatureManager.php

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


lib/private/Security/Signature/SignatureManager.php

apps/files_sharing/lib/External/Cache.php

apps/cloud_federation_api/lib/Controller/RequestHandlerController.php

+			// 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)) {


lib/private/Security/Signature/Model/Signatory.php

lib/private/Security/Signature/SignatureManager.php

+		if ($signatory === null) {
+			throw new SignatoryNotFoundException('empty result from getRemoteSignatory');
+		}
+		if ($signatory->getKeyId() !== $signedRequest->getKeyId()) {


lib/private/Security/Signature/SignatureManager.php

apps/cloud_federation_api/lib/Controller/RequestHandlerController.php

+		if (!str_contains($entry, '@')) {
+			throw new IncomingRequestException('entry does not contains @');
+		}
+		[, $instance] = explode('@', $entry, 2);


lib/private/Security/PublicPrivateKeyPairs/Model/KeyPair.php

+	 * @inheritDoc
+	 *
+	 * @param string $publicKey
+	 * @return IKeyPair


lib/private/Security/PublicPrivateKeyPairs/Model/KeyPair.php

+	 * @inheritDoc
+	 *
+	 * @param string $privateKey
+	 * @return IKeyPair


lib/private/Security/PublicPrivateKeyPairs/Model/KeyPair.php

+	 * @inheritDoc
+	 *
+	 * @param array $options
+	 * @return IKeyPair


lib/private/Security/Signature/Model/OutgoingSignedRequest.php

+	 * @inheritDoc
+	 *
+	 * @param string $host
+	 * @return IOutgoingSignedRequest


lib/private/Security/Signature/Model/OutgoingSignedRequest.php

+	 * @param string $key
+	 * @param string|int|float|bool|array $value
+	 *
+	 * @return IOutgoingSignedRequest


lib/private/Security/Signature/Model/OutgoingSignedRequest.php

+	 *
+	 * @param string $estimated
+	 *
+	 * @return IOutgoingSignedRequest


lib/private/Security/Signature/Model/OutgoingSignedRequest.php

+	 *
+	 * @param string $algorithm
+	 *
+	 * @return IOutgoingSignedRequest


lib/private/Security/Signature/Model/SignedRequest.php

+	 * @inheritDoc
+	 *
+	 * @param array $signatureHeader
+	 * @return ISignedRequest


lib/private/Security/Signature/Model/SignedRequest.php

+	/**
+	 * @inheritDoc
+	 *
+	 * @return ISignatory


lib/private/Security/Signature/Model/SignedRequest.php

+	 * @since 30.0.0
+	 */
+	public function getSignatory(): ISignatory {
+		return $this->signatory;


to.json

nickvergessen · 2024-07-11T07:44:15Z

apps/cloud_federation_api/lib/Controller/RequestHandlerController.php

+			// 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'] ?? '');


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?

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)

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

As said before, I only use this entry because it exists in the OCM protocol and doing so to compare that the origin of the reshare request, based on the token (and the linked recipient stored in the database), confirm the identity used to sign the request.

If Talk is using this endpoint to initiate anything, signature are to be required

apps/cloud_federation_api/lib/Controller/RequestHandlerController.php

lib/private/Federation/CloudFederationProviderManager.php

lib/public/Security/Signature/SignatureAlgorithm.php

lib/public/Security/Signature/Model/SignatoryType.php

lib/private/Security/Signature/Model/SignedRequest.php

lib/private/Security/Signature/SignatureManager.php

core/Migrations/Version30000Date20240101084401.php

nickvergessen · 2024-07-11T08:11:40Z

lib/private/Security/Signature/SignatureManager.php

+		}
+	}
+
+	private function insertSignatory(ISignatory $signatory): void {


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?

no real reason other that personal preference

lib/private/Security/Signature/SignatureManager.php

ArtificialOwl · 2024-11-18T08:40:40Z

Since last review, new commit:

switching to IdentityProof
removing features not fully related to this PR
fixing some edge use case on federationId,
fixing an issue to extract the origin of a notification on a reshare,
fixing test by adding threads to httpd,
fixing all comments/requests from previous reviews
upgrading version to trigger database migration

lib/private/Security/Signature/SignatureManager.php

Signed-off-by: Maxence Lange <[email protected]>

come-nc

There should be a mapper for signatory
There should not be a digest sent to openssl_* which is creating a digest by itself.
clearSignature should be renamed to something more appropriate.

come-nc · 2024-11-21T09:50:01Z

lib/private/Security/Signature/Model/IncomingSignedRequest.php

+		$date = $this->getRequest()->getHeader('date');
+		if ($date === '') {
+			throw new SignatureNotFoundException('missing date in header');
+		}
+		$contentLength = $this->getRequest()->getHeader('content-length');
+		if ($contentLength === '') {
+			throw new SignatureNotFoundException('missing content-length in header');
+		}
+		$digest = $this->getRequest()->getHeader('digest');
+		if ($digest === '') {
+			throw new SignatureNotFoundException('missing digest in header');
+		}
+		if ($this->getRequest()->getHeader('Signature') === '') {
+			throw new SignatureNotFoundException('missing Signature in header');
+		}


Suggested change

$date = $this->getRequest()->getHeader('date');

if ($date === '') {

throw new SignatureNotFoundException('missing date in header');

}

$contentLength = $this->getRequest()->getHeader('content-length');

if ($contentLength === '') {

throw new SignatureNotFoundException('missing content-length in header');

}

$digest = $this->getRequest()->getHeader('digest');

if ($digest === '') {

throw new SignatureNotFoundException('missing digest in header');

}

if ($this->getRequest()->getHeader('Signature') === '') {

throw new SignatureNotFoundException('missing Signature in header');

}

$date = $this->request->getHeader('date');

if ($date === '') {

throw new SignatureNotFoundException('missing date in header');

}

$contentLength = $this->request->getHeader('content-length');

if ($contentLength === '') {

throw new SignatureNotFoundException('missing content-length in header');

}

$digest = $this->request->getHeader('digest');

if ($digest === '') {

throw new SignatureNotFoundException('missing digest in header');

}

if ($this->request->getHeader('Signature') === '') {

throw new SignatureNotFoundException('missing Signature in header');

}

come-nc · 2024-11-21T09:50:35Z

lib/private/Security/Signature/Model/IncomingSignedRequest.php

+		// confirm presence of date, content-length, digest and Signature
+		$date = $this->getRequest()->getHeader('date');
+		if ($date === '') {
+			throw new SignatureNotFoundException('missing date in header');


SignatureNotFoundException does not feel like the right exception here?

All thrown in this method should be IncomingRequestException I think

come-nc · 2024-11-21T09:53:02Z

lib/private/Security/Signature/Model/IncomingSignedRequest.php

+			}
+
+			[$k, $v] = explode('=', $entry, 2);
+			preg_match('/"([^"]+)"/', $v, $var);


Suggested change

preg_match('/"([^"]+)"/', $v, $var);

preg_match('/^"([^"]+)"$/', $v, $var);

I suppose, no?

I know mine is working, what should I expected different from yours ?

https://regex101.com/r/BPNH0j/1
Yours will match quotes even if there is unexpected stuff around.
Mine makes sure that quotes are first and last characters.

lib/private/Security/Signature/Model/OutgoingSignedRequest.php

come-nc · 2024-11-21T09:57:41Z

lib/private/Security/Signature/Model/OutgoingSignedRequest.php

+	 * @inheritDoc
+	 *
+	 * @param string $host
+	 * @return IOutgoingSignedRequest


Suggested change

* @return IOutgoingSignedRequest

* @return $this

You should use this for fluent interfaces in phpdoc.
Also you do not need it in the class when it’s there on the public interface it’s inherited by default.

come-nc · 2024-11-21T11:10:17Z

lib/private/Security/Signature/Model/SignedRequest.php

+		private readonly string $body,
+	) {
+		// digest is created on the fly using $body
+		$this->digest = 'SHA-256=' . base64_encode(hash('sha256', utf8_encode($body), true));


utf8_encode is deprecated.
Do you really have the body in ISO-8859-1 here? Why?

come-nc · 2024-11-21T11:13:16Z

lib/private/Security/Signature/SignatureManager.php

+			$estimated[] = $key . ': ' . $value;
+		}
+
+		$signedRequest->setClearSignature(implode("\n", $estimated));


I still dislike that this is stored in the IIncomingSignedRequest while computed outside. Either there is only getClearSignature that computes the value, or the method is only in the signature manager and returns it, no need to set it on the object.

Also, I’m pretty sure this should be named an other way. "clearSignature" is confusing. But I’m not sure what is the common term for this. fingerprint? signatureData? signedData? requestData?

come-nc · 2024-11-21T11:19:37Z

lib/private/Security/Signature/Model/SignedRequest.php

+		private readonly string $body,
+	) {
+		// digest is created on the fly using $body
+		$this->digest = 'SHA-256=' . base64_encode(hash('sha256', utf8_encode($body), true));


Actually you should not compute a digest yourself. This is the job of openssl_sign and openssl_verify, and why you give them a sum algorithm to use.

come-nc · 2024-11-21T11:20:34Z

lib/private/Security/Signature/SignatureManager.php

+		$knownSignatory = $this->getStoredSignatory($signatory->getKeyId());
+		switch ($signatory->getType()) {
+			case SignatoryType::FORGIVABLE:
+				$this->deleteSignatory($knownSignatory->getKeyId());
+				$this->insertSignatory($signatory);
+				return;
+
+			case SignatoryType::REFRESHABLE:
+				$this->updateSignatoryPublicKey($signatory);
+				$this->updateSignatoryMetadata($signatory);
+				break;
+
+			case SignatoryType::TRUSTED:
+				// TODO: send notice to admin
+				throw new SignatoryConflictException();
+
+			case SignatoryType::STATIC:
+				// TODO: send warning to admin
+				throw new SignatoryConflictException();
+		}


A bit overkill to have these 4 types, are they all used and needed? Is it an admin choice?

it is set by the 3rd party ISignatoryManager that will decide how must be handle a change of the remote public key

Signed-off-by: Maxence Lange <[email protected]>

sorbaugh · 2024-11-26T10:26:02Z

Hello, due to the importance of having this issue merged, please let's postpone all approach-related optimization to later. If this PR checks this boxes, please let's approve and merge:

PR fixes the issue / adds the feature as intended
PR is backport compatible (needs a config switch that's OFF by default, needs to work with the OCM ecosystem)

After that we should update the documentation about the config switch.

Thanks! 🙏

github-advanced-security bot found potential problems Jun 19, 2024

View reviewed changes

github-advanced-security bot found potential problems Jun 25, 2024

View reviewed changes

ArtificialOwl force-pushed the enh/noid/signed-request branch from 58d3efd to f45a2cb Compare July 2, 2024 04:46

github-advanced-security bot found potential problems Jul 2, 2024

View reviewed changes

ArtificialOwl force-pushed the enh/noid/signed-request branch from f45a2cb to a3dda22 Compare July 4, 2024 16:03

github-advanced-security bot found potential problems Jul 4, 2024

View reviewed changes

ArtificialOwl force-pushed the enh/noid/signed-request branch from 61cc09b to 9ef90c0 Compare July 10, 2024 12:01

github-advanced-security bot found potential problems Jul 10, 2024

View reviewed changes

ArtificialOwl added the 3. to review Waiting for reviews label Jul 10, 2024

ArtificialOwl added this to the Nextcloud 30 milestone Jul 10, 2024

ArtificialOwl marked this pull request as ready for review July 10, 2024 16:24

ArtificialOwl requested a review from nickvergessen as a code owner July 10, 2024 16:24

nickvergessen reviewed Jul 11, 2024

View reviewed changes

to.json Outdated Show resolved Hide resolved

nickvergessen requested changes Jul 11, 2024

View reviewed changes

This was referenced Jul 30, 2024

30.0.0 beta 2 #46870

Merged

30.0.0 beta 3 #46946

Merged

This was referenced Aug 5, 2024

30.0.0 beta 4 #47028

Merged

30.0.0 beta 5 #47100

Merged

skjnldsv mentioned this pull request Aug 13, 2024

30.0.0 RC1 #47190

Merged

skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 14, 2024

ArtificialOwl mentioned this pull request Aug 21, 2024

Signing request cs3org/OCM-API#92

Merged

michielbdejong mentioned this pull request Sep 20, 2024

feat(CloudFederationApi): Start implementation of 1.1.0 of OCM spec #47199

Draft

13 tasks

skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 29, 2024

ArtificialOwl force-pushed the enh/noid/signed-request branch 4 times, most recently from 444f9e7 to f3a1684 Compare October 31, 2024 12:21

ArtificialOwl requested a review from provokateurin as a code owner October 31, 2024 12:45

ArtificialOwl force-pushed the enh/noid/signed-request branch 10 times, most recently from df8f30e to 1b02a3c Compare November 18, 2024 00:56

ArtificialOwl added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 18, 2024

come-nc reviewed Nov 18, 2024

View reviewed changes

lib/private/Security/Signature/SignatureManager.php Outdated Show resolved Hide resolved

come-nc reviewed Nov 18, 2024

View reviewed changes

lib/private/Security/Signature/SignatureManager.php Outdated Show resolved Hide resolved

come-nc reviewed Nov 18, 2024

View reviewed changes

lib/private/Security/Signature/SignatureManager.php Outdated Show resolved Hide resolved

come-nc reviewed Nov 18, 2024

View reviewed changes

lib/private/Security/Signature/SignatureManager.php Outdated Show resolved Hide resolved

ArtificialOwl force-pushed the enh/noid/signed-request branch 5 times, most recently from c7683aa to bcb284a Compare November 21, 2024 03:37

ArtificialOwl added 3 commits November 21, 2024 09:26

feat(ocm): signing ocm requests

62b9957

Signed-off-by: Maxence Lange <[email protected]>

fix(ocm): switching to IdentityProof

e900995

Signed-off-by: Maxence Lange <[email protected]>

fix(ocm): simpler code

ab72b03

Signed-off-by: Maxence Lange <[email protected]>

ArtificialOwl force-pushed the enh/noid/signed-request branch from bcb284a to ab72b03 Compare November 21, 2024 10:26

come-nc requested changes Nov 21, 2024

View reviewed changes

feat(signatory): switch to qbmapper

618ff23

Signed-off-by: Maxence Lange <[email protected]>

ArtificialOwl force-pushed the enh/noid/signed-request branch from c00446b to 618ff23 Compare November 22, 2024 17:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Signed requests #45979

Signed requests #45979

ArtificialOwl commented Jun 19, 2024 •

edited

Loading

nickvergessen Jul 11, 2024

ArtificialOwl Jul 11, 2024

nickvergessen Jul 11, 2024

ArtificialOwl Oct 30, 2024

nickvergessen Jul 11, 2024

ArtificialOwl Jul 11, 2024

ArtificialOwl commented Nov 18, 2024

come-nc left a comment

come-nc Nov 21, 2024

come-nc Nov 21, 2024

come-nc Nov 21, 2024

come-nc Nov 21, 2024

ArtificialOwl Nov 22, 2024

come-nc Nov 25, 2024

come-nc Nov 21, 2024

come-nc Nov 21, 2024

come-nc Nov 21, 2024

come-nc Nov 21, 2024

come-nc Nov 21, 2024

ArtificialOwl Nov 22, 2024

sorbaugh commented Nov 26, 2024

	preg_match('/"([^"]+)"/', $v, $var);
	preg_match('/^"([^"]+)"$/', $v, $var);

Signed requests #45979

Are you sure you want to change the base?

Signed requests #45979

Conversation

ArtificialOwl commented Jun 19, 2024 • edited Loading

Signed Request

Signature

ISignatoryManager

ISignatureManager

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArtificialOwl commented Nov 18, 2024

come-nc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorbaugh commented Nov 26, 2024

ArtificialOwl commented Jun 19, 2024 •

edited

Loading