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

fix: Fix PSD2 based cabfOrganizationIdentifier check #880

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

XolphinMartijn
Copy link
Contributor

This fix allows for PSD2 based cabfOrganizationIdentifier extensions to both include the NCA identifier in the registrationReference, as well as leaving it out.

This is an item recently discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=1897538 in which we have stated why it's our interpretation that the NCA is not supposed to be part of the registrationReference. The fix leaves the option to keep the NCA in the registrationReference as a valid option as well,since several other CAs are using that as default practice. Until there is a change in the EVGs, we suggest keeping both options as an allowed method in zlint

Copy link
Member

@christopher-henderson christopher-henderson left a comment

Choose a reason for hiding this comment

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

Thank you @XolphinMartijn!

I had found one issue in the use of mutable global state in this PR. However, it prompted me to look more closely at this file and I would like to offer a small refactor to help us ensure that we are correct on the semantics of this issue.

The specific issue was the reuse of a single OrganizationIdentifier pointer across multiple parses.

  1. Multiple parses can easily lead to structs left in a half stale state.
  2. OrganizationIdentifier has effectively two “modes”
    2a. Parse as PSD and Parse not as PSD
  3. Thus, keeping state consistent across multiple parses-and-modes on a single object is quite a few branches.

It was honestly making it somewhat difficult for me to prove that there wasn’t a bug in this lint. I would appreciate it if you can take a look at a small refactor I did on this branch.

I opened up a PR against your fork. If you’re amenable, then please take a look.

@XolphinMartijn
Copy link
Contributor Author

@christopher-henderson Thank you. Reviewed and merged the PR on my repo

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.

2 participants