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.

Sign up for GitHub

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

Jump to bottom

docs: audit notes #91

Merged
merged 2 commits into from
Oct 31, 2023
Merged

docs: audit notes #91

merged 2 commits into from
Oct 31, 2023

Conversation

AaronFeickert
Copy link
Contributor

@AaronFeickert AaronFeickert commented Oct 13, 2023 •
edited
Loading

This adds the Quarkslab audit report and a brief list of responses.

@AaronFeickert AaronFeickert marked this pull request as draft October 16, 2023 14:34
hansieodendaal
hansieodendaal reviewed Oct 18, 2023
View reviewed changes
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

Looking good, but I think we should update curve25519-dalek. The version we chose was just temporary to get the ledger stuff going.

docs/quarkslab-audit/README.md Outdated
Comment on lines 39 to 40
The fork may be updated in the future against a more recent upstream tag if doing so does not introduce conflicts in Tari repositories.
Further, an open [upstream pull request](https://github.com/dalek-cryptography/curve25519-dalek/pull/546) would add partial precomputation support; if it is accepted, the implementation may change its curve library dependency to upstream.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should update to their (curve25519-dalek) latest version and apply our specific changes to it in our fork, similar to what we did in the past.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certainly can. Upstream switched to a monorepo structure after the release candidates, but I doubt this would cause issues other than pulling in a lot of functionality that wouldn't be needed. I'll test this out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in the curve library fork by this PR.

@AaronFeickert AaronFeickert force-pushed the audit-notes branch 4 times, most recently from 3472175 to a1ced35 Compare October 25, 2023 18:21
@AaronFeickert AaronFeickert marked this pull request as ready for review October 25, 2023 18:23
hansieodendaal
hansieodendaal reviewed Oct 26, 2023
View reviewed changes
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

Just a small comment below, and then I was wondering, if would it be beneficial to list the PRs we introduced that addressed all the concerns the auditors had, the ones we did due to the "tips" we received during the audit and the ones we did to address the formal findings? Then a reader could look at the audited commit and the relevant PRs leading up to the final commit after the audit to put everything in context.

README.md Outdated Show resolved Hide resolved
CjS77
CjS77 approved these changes Oct 26, 2023
View reviewed changes
Copy link
Contributor

@CjS77 CjS77 left a comment

Choose a reason for hiding this comment

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

If you add in the commit for the review, I'm happy to merge.

WRT specific PRs, iirc there's a post-review by the auditors that could give a note saying "all issues resolved", which would be first prize.

@AaronFeickert
Copy link
Contributor Author

The lint-related changes recommended by the auditors were made over the course of several PRs, many of which also addressed other things like refactoring or efficiency. I'm not sure how useful it would be for the reader to list them all.

I'll add a link to the audit commit/tag.

hansieodendaal
hansieodendaal approved these changes Oct 26, 2023
View reviewed changes
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice

ACK

@CjS77 CjS77 merged commit 462ba72 into tari-project:main Oct 31, 2023
6 checks passed
@AaronFeickert AaronFeickert deleted the audit-notes branch November 1, 2023 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hansieodendaal hansieodendaal hansieodendaal approved these changes

@CjS77 CjS77 CjS77 approved these changes

Assignees
No one assigned
Labels
CR-insufficient_context P-acks_required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AaronFeickert @CjS77 @hansieodendaal @ghpbot-tari-project