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

Pallet AutoId #2641

Merged
merged 9 commits into from
Apr 2, 2024
Merged

Pallet AutoId #2641

merged 9 commits into from
Apr 2, 2024

Conversation

vedhavyas
Copy link
Member

@vedhavyas vedhavyas commented Mar 26, 2024

This PR introduces pallet-auto-id with following functionality

  • Register AutoId using X509 cert
  • Revoke certificate
  • Deactivate AutoId

This is not the full implementation of the as per spec. Current changes will get us started with above functionality. Once we have a new runtime and domain node, I'm planning to add the remaining spec post that.

I have also left some TODOs regading the revocation and deactivation. Once we have some feedback, based on the feasibility of implementation, they will be handled in the seperate PR

cc: @jfrank-summit

Code contributor checklist:

Copy link

gitguardian bot commented Mar 26, 2024

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
10089939 Triggered Generic Private Key a11c528 domains/pallets/auto-id/res/private.issuer.pem View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secret safely. Learn here the best practices.
  3. Revoke and rotate this secret.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

@vedhavyas vedhavyas changed the title add primitive crate for signature verification Pallet AutoId Mar 26, 2024
@vedhavyas vedhavyas added the need to audit This change needs to be audited label Mar 26, 2024
@vedhavyas vedhavyas force-pushed the certificate_registry branch from cfa0c35 to a88badb Compare March 26, 2024 06:55
@vedhavyas vedhavyas force-pushed the certificate_registry branch from a88badb to a11c528 Compare March 26, 2024 07:03
Copy link
Member

@jfrank-summit jfrank-summit left a comment

Choose a reason for hiding this comment

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

I gave an initial pass and and left some comments regarding the general approach.

@vedhavyas vedhavyas requested a review from jfrank-summit March 28, 2024 05:18
Copy link
Member

@jfrank-summit jfrank-summit left a comment

Choose a reason for hiding this comment

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

I had one nit and I still have some questions on overall design but we can address them later in order to begin testing sooner. @NingLin-P should also review.

jfrank-summit
jfrank-summit previously approved these changes Apr 1, 2024
Copy link
Member

@jfrank-summit jfrank-summit left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@NingLin-P NingLin-P left a comment

Choose a reason for hiding this comment

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

Rest LGTM

Comment on lines 363 to 369
let req = SignatureVerificationRequest {
public_key_info: auto_id.certificate.subject_public_key_info(),
signature_algorithm,
// uses auto_id identifier as the message to sign
data: auto_id.identifier.encode(),
signature,
};
Copy link
Member

Choose a reason for hiding this comment

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

This seems problematic, since the signing data is always the same the signature is always valid, so the attacker can reuse the signature from previous blocks to construct arbitrary request.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!
We would need some extra random data for this block for users to sign along with AutoId, we can previous block hash. UX issue with this, whichever data we pick, transaction is valid only for one block. Users need to re-sign if the transaction does not make it into the block.
Thoughts @jfrank-summit

Copy link
Member

Choose a reason for hiding this comment

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

We would need some extra random data for this block for users to sign along with AutoId, we can previous block hash

This may not be enough as the attacker can listen to the tx pool and act something similar to MEV to front run the normal request. Ideally, we should sign the register/revoke/deactivate request itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem still exists in that case as well since there is no specific request data for these actions.
I would need to think more this

Copy link
Member Author

@vedhavyas vedhavyas Apr 1, 2024

Choose a reason for hiding this comment

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

Actually, we can solve this with a nonce and request specific data. So even if the attacker does MEV, this signature will only be valid for specific request and specific nonce. They will end up paying for it instead of user.

Request specific data can be as simple as a enum variant. But this introduces signing nonce for each certificate operation.
cc: @jfrank-summit

Copy link
Member

Choose a reason for hiding this comment

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

What does the user flow look like for this? Will we iterate nonces or track used random nonces?

Copy link
Member Author

Choose a reason for hiding this comment

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

every certificate will have a signing nonce like we have a nonce for the account. Along with the nonce, we also will have the request specific pre-defined data. So the final data to be signed would look like this

Action enum {
	RevokeCert
	DeactivateAutoId
	...
}

SigningData struct {
	ID
	Nonce
	Action
}

This ensures, the signature is valid for this (nonce. action) and cannot be re-used.
This does not affect the X509 certificate signing. Just the actions done by the subject of the certificate

@vedhavyas vedhavyas force-pushed the certificate_registry branch from a825753 to 5de3054 Compare April 2, 2024 03:53
@vedhavyas vedhavyas enabled auto-merge April 2, 2024 03:53
@vedhavyas vedhavyas added this pull request to the merge queue Apr 2, 2024
Merged via the queue into main with commit 4068a08 Apr 2, 2024
9 checks passed
@vedhavyas vedhavyas deleted the certificate_registry branch April 2, 2024 10:58
@vanhauser-thc
Copy link
Collaborator

for PRs that are to be audited, we need context.
What is autoid, what are the specs etc? can you please submit links or a good description?
thank you.

@jfrank-summit jfrank-summit removed the need to audit This change needs to be audited label Apr 8, 2024
@jfrank-summit
Copy link
Member

@vanhauser-thc The needs to audit label on this PR was premature as we are still defining specs, etc.

@vedhavyas
Copy link
Member Author

@vanhauser-thc yes, I tagged the label prematurely. Will reach back once we have more specs in place for your audit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants