-
Notifications
You must be signed in to change notification settings - Fork 43
Async enrollment postgres #12002
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
Async enrollment postgres #12002
Conversation
b3a7e3c to
1475f27
Compare
1475f27 to
33d94da
Compare
54a5869 to
a0122c1
Compare
33d94da to
b475f78
Compare
a0122c1 to
bf6df65
Compare
b475f78 to
6af9a44
Compare
| """) | ||
|
|
||
|
|
||
| _q_insert_user_and_device = Q(f""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For pki_accept this was implemented re-using _q_insert_user_and_device from user_create_user.py:
| from parsec.components.postgresql.user_create_user import _q_insert_user_and_device |
I know that we've started to prefer each component/command to define its own set of queries (specially when there are slightly differences between the needs of each command), BUT for complex queries like this one, the duplication introduces a significant risk of error IMO.
Since there seems to be no difference, why not reusing-it? The only potential issue I see is that it has to be clear when a query can be reused... we could move them to the queries directory which already contains "common" queries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are totally right on this, I've updated the code to directly use _q_insert_user_and_device 👍
| # - Should not be longer than 10 items (including leaf and root) | ||
| # - Each intermediate certificate should be used exactly once | ||
|
|
||
| if len(intermediate_der_x509_certificates) > 8: # 8 since we exclude leaf and root |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the MAX_INTERMEDIATE_CERTIFICATES_DEPTH (the value is up to discussion)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, this also makes simpler to test this error case
| return f"{self.__class__.__qualname__}(subject={self.subject}, issuer={self.issuer})" | ||
|
|
||
|
|
||
| def _validate_x509_trustchain( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to have a look at build_trustchain
parsec-cloud/server/parsec/components/pki.py
Line 331 in b68046e
| async def build_trustchain( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a look at it, and think it would be simpler to avoid using dictionary when checking the intermediary certificates:
- the number of items is very small (and async enrollement APIs are not used often), so there is no performance concern
list(trustchain.values())relies on the fact Python dictionaries retain insertion order, this is something implicit (and it has not been always the case), so I'd rather avoid it
On top of that, the fact PkiCertificate gets its signed_by attribute updated add complexity (since the object need to be mutable) and is not strictly needed (the fingerprint can be retrieved by iterating over the trustchain in the code doing the SQL query, which means quadratic complexity but again this is not an issue since the number of certificates is small)
7059f1f to
c20c78b
Compare
a5f17d6 to
1b83563
Compare
…in `PGEventsComponent`
…ommon_(write|read)` functions in server postgresql code
…ki` server fixture
…ubmitting async enrollement
1b83563 to
17e4257
Compare
No description provided.