-
Notifications
You must be signed in to change notification settings - Fork 10
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
Implement RFC8738 IP Identifier Validation Support #12
base: master
Are you sure you want to change the base?
Conversation
Hi!
first off, thanks a lot for putting in the work. I'm not in the office
this week, but I do have some notes, though:
* this kind of feature should be gated behind a config setting. i'd like
to have a key `identifierTypes` that defaults to `dns` only, and the
admin must explicitly enable `ip`s. this is because this doesn't mesh
well with the `verifyPTR` (et.al) restrictions we allow to place on
potential acme clients.
* by introducing the ChallengeIdentifier class (replacing plain `str`s)
the interface between serles-core and the backends change. as you
probably noticed, that forced you to modify the code in a bunch of
places. we must avoid this change at all costs, as this interface is
publicly documented and external/private backends (which we know
exist) depend on this API not to change.
essentially, this class implements a named tuple for ident type and
value; we already track that information in the DB/models Identifier
class. there are also a bunch of seemingly redundant isinstance checks
* as a consequence of not breaking API, the backend-specific stuff e.g.
happening in ejbca_identifier() must be contained within that backend.
* as far as i can see, the rfc only allows ip *addresses*, not networks!
* the spec is also pretty keen on making sure that the formatting of ip
addresses is well-defined - this needs to be ensured by the code.
* §6 talks about some special requirements in the alpn challenge, which
we need to handle.
* please keep this PR focused on the feature and don't add type hints at
the same time. (i personally am not very fond of them, since they add
a lot of noise. independent of that, it makes diffs larger and
following them harder)
* i know this is still wip, but just so we don't forget: this feature
needs tests (we aim for 100% code coverage through unit tests)
* if you can, i'd prefer rebased commits between reviews :)
* note to self: the new models.IdentifierType entry causes a change in
the generated SQL schema. this is unavoidable. we must document that
properly in changelog, because users might need to `ALTER TABLE
identifier`!
please don't be discouraged by the length of my notes - adding this
feature requires us to remove a number of implicit assumptions in the
code base. I'm sure we can work though this together :)
|
I know your concern and requirements now and I have a clue about how to design that. However, I am a little bit confuse about how not to break the backend API interface. Or we just pass all the value as str and let the backend to test if they are IP address or not.
The |
I found that cryptography support decode/encode ipaddress general name with netmask in pyca/cryptography/src/rust/src/x509/common.rs#L277 Since the CSR is parsed from user upload data, we should handle it by either converting it to ipaddress or returning an error to user. |
Or we just pass all the value as str and let the backend to test if they are IP address or not.
yes, you'll have to pass strings around and check their contents in the backend(s).
The cryptography library said it can return network type (I am also confused about that), therefore, I added the conversion for that case.
they support a lot of stuff that's not covered by ACME :^) let's stick to adresses only.
Since the CSR is parsed from user upload data, we should handle it by either converting it to ipaddress or returning an error to user.
we check the requested identifiers earlier, when creating the order. no need to check the csr, since if it contains additional identifiers, we will reject it anyways
Message ID: ***@***.***>
|
Here is the PR for supporting RFC8738 IP Identifier Validation Support.
I added a new type called
ChallengeIdentifier
to ensure consistency of comparing different types of identifiers.I also added some Python typing to make the codes clearer and make editor auto completion to work correctly.
I haven't tested it yet, so I mark this PR as WIP.
Close: #11