-
Notifications
You must be signed in to change notification settings - Fork 9
Digital signatures (IVS-499 and IVS-500) #190
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
Conversation
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.
Mostly clerical comments on my end. Looks like a solid MVP!
backend/apps/ifc_validation_models
Outdated
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.
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 don't see this 0012 on the remote. Where does it come from:
https://github.com/buildingSMART/ifc-validation-data-model/tree/main/migrations
I do remember there was something funky when I authored the migration. So I probably messed up, but I'm unsure how to correct.
Can you do a git log 0012_alter_useradditionalinfo ...
for me to see where that file was introduced? And maybe cherry-pick and push that commit again.
But also going from the validate/dev remote to the submodule I only get to 0011 migrations.
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.
So, 0012 was not in source control anywhere, just in my local working tree. IIRC there was an issue with 0011 that django flagged and offered to auto-fix. 0012 is the resulting fix. Probably safest to include it and then make the digital signatures into migration 0013.
.gitmodules
Outdated
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.
#TODO: update developer documentation that a local instance of the Validation Service requires an additional submodule.
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.
from enum import Enum, auto | ||
|
||
# @nb These (rather incomplete) bindings are no | ||
# longer needed, we just use the openssl executable |
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.
# longer needed, we just use the openssl executable |
|
||
# @nb These (rather incomplete) bindings are no | ||
# longer needed, we just use the openssl executable | ||
# from asn1crypto import cms |
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.
# from asn1crypto import cms |
} | ||
|
||
|
||
class ca_bundle: |
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.
CapWords convention. It would be helpful to be more explicit here.
class ca_bundle: | |
class CertAuthorityBundle: |
backend/apps/ifc_validation/checks/signatures/test_files/pass_multiple_signatures.ifc
Outdated
Show resolved
Hide resolved
backend/apps/ifc_validation/tasks.py
Outdated
check_program = [sys.executable, check_script, file_path] | ||
logger.debug(f'Command for {self.__qualname__}: {" ".join(check_program)}') | ||
|
||
# check schema |
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.
# check schema | |
# check signatures |
Possibly a holdover from the bSDD task?
@@ -820,17 +907,13 @@ def bsdd_validation_subtask(self, prev_result, id, file_name, *args, **kwargs): | |||
@shared_task(bind=True) | |||
@log_execution | |||
@requires_django_user_context | |||
@update_progress |
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.
Nice refactoring, BTW
frontend/src/DashboardTable.js
Outdated
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.
No comments on front end other than a collective note to our future selves to add the new icons to the documentation and explain what each one corresponds to (no signature, invalid signature, valid signature).
Thanks for the review @civilx64 everything is addressed minus migration conflict, so I'm not going to merge yet. Also sphinx docs not updated yet. |
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 believe we solved the mystery of the missing migration. If you agree w/ the digital signatures becoming migration 0013 then we are good to merge.
Ok, so your 0012 adds
I guess that's not unreasonable even if we don't know exactly where it comes from. I've harmonized the migrations and merged this in. |
Note that there is one part of the puzzle missing: verify that the authoring tool in the header matches the subject/issuer of the certificate. This is not very urgent because we do verify that it's a certificate that we trust and there is no immediate reason to suspect Autodesk signing Graphisoft files (they shouldn't have the private key). Not doing this buys us some time to figure out basic things incrementally: figure out which distinguished name fields we're interested in; how strict we are with certain properties of the cert (key size, algos, etc.); how many certs per vendor; validity time frame and revocation.