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

Add certificates store validation #1899

Closed
Tracked by #1902
leblowl opened this issue Oct 3, 2023 · 5 comments
Closed
Tracked by #1902

Add certificates store validation #1899

leblowl opened this issue Oct 3, 2023 · 5 comments
Assignees
Labels
bug Something isn't working security

Comments

@leblowl
Copy link
Collaborator

leblowl commented Oct 3, 2023

Currently anyone can write to the certificates log. This allows anyone to forge certificates. certificates store should only be writable to by the owner and we should validate certs and verify they are signed by the owner.

With the additions of #1843, when certificates are replicated:

this.certificates.events.on('replicated', async () => {
this.logger('REPLICATED: Certificates')
this.emit(SocketActionTypes.CONNECTION_PROCESS_INFO, ConnectionProcessInfo.CERTIFICATES_REPLICATED)
this.emit(StorageEvents.LOAD_CERTIFICATES, {
certificates: this.getAllEventLogEntries(this.certificates),
})
await this.updatePeersList()

They are then sent to the client, where they are parsed and added to the Redux store:

responseSendCertificates: (state, action: PayloadAction<SendCertificatesResponse>) => {
certificatesAdapter.setAll(
state.certificates,
Object.values(action.payload.certificates).map(item => {
if (!item) {
return
}
return parseCertificate(item)
})
)
},

parseCertificate doesn't check the signature.

Then certs are used for usernames via allUsers selector:

export const allUsers = createSelector(csrsMapping, certificatesMapping, (csrs, certs) => {

If we don't verify that a cert is signed by the owner, we cannot guarantee username uniqueness and we cannot guarantee the public key in the cert is connected to the username in the cert... assuming that we trust the owner to check uniqueness and verify CSR signatures.

@holmesworcester
Copy link
Contributor

holmesworcester commented Oct 3, 2023

Right now lucas believes this means that anyone can change another user's name.

@holmesworcester
Copy link
Contributor

We should address #1891 first and then address this.

@leblowl
Copy link
Collaborator Author

leblowl commented Oct 5, 2023

We can assume some of the the infrastructure necessary for this task will be implemented in #1891. Specifically that there will exist functions putOwnerOrbitDbIdentityId and getOwnerOrbitDbIdentityId in src/nest/storage/identity.ts. If implementing this in parallel to #1891, we can mock these functions and use them to verify that only the owner can append entries to the certificates store. We can use OrbitDB's IPFSAccessController as an example of how to do that: https://github.com/orbitdb/orbit-db-access-controllers/blob/3741eb318e9c7efea5af15a54110cf6de8ae1fe3/src/access-controllers/ipfs.js#L20

We should also verify:

  • that the certificate is valid (parses correctly, formatted correctly, contains required fields, etc.)
  • that the certificate is signed by the owner's key
  • that the entry key matches the certificates public key

Since it looks like access controllers only validate heads, I think we should simply validate each entry ourselves. We can create a new validation function validateCertificateEntry that, given an entry, validates the entry according the specs above (and calls Entry.verify) and returns true/false. Then we can create a new function, getCertificates that iterates over the certificates log entries, and filters the logs with validateCertificateEntry such that it only returns valid entries. It should also filter and return only the latest certificate per public key such that there is a 1-to-1 mapping of public key to cert. We can then use getCertificates to send valid certs to other parts of the application for processing. See also #1893. We can optimize this later.

It might be nice to encapsulate everything in a CertificateStore class. See also: #1923

@leblowl leblowl changed the title Only the owner should be able to write to certificates store Add certificates validation Oct 7, 2023
@leblowl leblowl changed the title Add certificates validation Add certificates store validation Oct 7, 2023
@siepra siepra moved this from Sprint to In progress in Quiet Nov 14, 2023
@siepra siepra self-assigned this Nov 14, 2023
@siepra siepra moved this from In progress to Waiting for review in Quiet Nov 28, 2023
siepra added a commit that referenced this issue Nov 29, 2023
* chore: start moving certificates to its own store

* chore: postpone validation work

* chore: move certificates to its own store

* fix: checking username availability

* test: fix

* fix: lint

* test: temporarily remove empty spec files

* fix: resolving promises on certificates loading

* fix: updating peer list

* chore: remove leftover

* test: add certificates store unit tests

* feat: validate certificates against the authority and format #1899

* chore: Merge chore/validate-scrs into chore/1899

* chore: Revert "chore: Merge chore/validate-scrs into chore/1899"

This reverts commit 73c4f8d.

* chore: update CHANGELOG.md

* test: verify certificate against the authority (negative case)

* fix: lint

* bug: updatePeersList returning wrong data type

* fix: update localdb peers list

* chore: Revert "fix: update localdb peers list"

This reverts commit 64d64c0.

* test: unskip write event test

* test: remove deprecated and broken cases

* fix: disable broken test

---------

Co-authored-by: Vin Kabuki <[email protected]>
@siepra siepra moved this from Waiting for review to Merged (develop) in Quiet Nov 29, 2023
@Kacper-RF Kacper-RF moved this from Merged (develop) to Ready for QA in Quiet Dec 1, 2023
@kingalg
Copy link
Collaborator

kingalg commented Dec 7, 2023

@leblowl any way I can check this doing manual checks?

@kingalg
Copy link
Collaborator

kingalg commented Jan 11, 2024

Desktop: [email protected]
Mobile: [email protected], ios 344

@kingalg kingalg closed this as completed Jan 11, 2024
@kingalg kingalg moved this from Ready for QA to Done in Quiet Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working security
Projects
Archived in project
Development

No branches or pull requests

4 participants