fix(HMS-3152): LocationName regex to allow uppercase #21
Conversation
|
From RFC 1123 section 2: So, the regex should support digit in intial position of |
public.openapi.yaml
Outdated
| @@ -979,7 +979,7 @@ components: | |||
| type: string | |||
| maxLength: 63 | |||
| minLength: 1 | |||
| pattern: ^[a-z][a-z0-9\-]*$ | |||
| pattern: ^[a-zA-Z][a-zA-Z0-9\-]*$ | |||
There was a problem hiding this comment.
Per RFC 1123, first character can be a digit. Also, the last character must be a letter or digit, not -.
There was a problem hiding this comment.
Just for reference, similar utility method in IPA: https://github.com/freeipa/freeipa/blob/master/ipalib/util.py#L389
There was a problem hiding this comment.
Looking at the RFC 1123 and RFC 952; If I understand correctly, the corrections indicated by RFC 1123 would match the below:
<hname> ::= <name>*["."<name>]
<name> ::= <let-or-digit>[*[<let-or-digit-or-hyphen>]<let-or-digit>]
Based on the above, the current change "^[a-zA-Z0-9]([a-zA-Z0-9\\-]*[a-zA-Z0-9])*$" would fit the above; but I have a question:
- I think is correct the change, but should
_be considered? For me it is not the same underscore_and hyphen-; please correct me if I were understanding wrongly the RFC; just in case_should be considered.
I have tried some edge cases at: https://regex101.com/
a99a9-fail => it is oka-fail => it is ok3-4-2fail => it is ok3A8A
There was a problem hiding this comment.
I was also thinking that we could allow any string and not have a regex here. We get the information from IPA server - IPA locations. So from that PoV we should support what it supports.
I just tried to define some random locations and it allows various other chars. It looks that the IPA location name is not much restricted even though it is defined as DNSNameParam.
There was a problem hiding this comment.
It looks that the IPA location name is not much restricted even though it is defined as
DNSNameParam.
hmmm... so there exists a real possibility that a customer could have non-conformant location data, which would cause registration failure.
I am OK with completely relaxing the location validation in our service. Bad data might (or might not, depending how their DNS is configured) be an operational issue for the IPA server. But I think we should not make the bad data prevent registration with idmsvc.
However, let's bring the issue up with the IPA team before committing. They might agree it's a bug and decide to properly restrict the field. Or we might be missing the reason why invalid DNS labels are allowed by IPA.
There was a problem hiding this comment.
I've opened #23 that is removing the pattern - so that we can choose which PR to merge.
As DNS labels are case insensitive and RFC 1035 allows upper case. Last character must be a digit or letter. Also per RFC 1123 section 2, the first character can be a digit. Before this change the regex was more restricitive than what was possible to define in IPA location. Signed-off-by: Petr Vobornik <[email protected]>
pvoborni
force-pushed
the
location-name-regex
branch
from
4c0f91b
to
e68cd76
Compare
Location name should be a DNS label and could be matched by a regex. Correct one could be "^[a-zA-Z0-9]([a-zA-Z0-9\\-]*[a-zA-Z0-9])*$". PR podengo-project#21 has a discussion about it. But users can defined more values in IPA location in the FreeIPA server than what the RFCs allow. From this PoV, it is more important for idmsvc to pass registration even with slightly invalid DNS label than being valid but failing. Idmsvc doesn't use the value for DNS operations. Signed-off-by: Petr Vobornik <[email protected]>
|
I think #23 is the one we should land - closing this one. |
Location name should be a DNS label and could be matched by a regex. Correct one could be "^[a-zA-Z0-9]([a-zA-Z0-9\\-]*[a-zA-Z0-9])*$". PR podengo-project#21 has a discussion about it. But users can defined more values in IPA location in the FreeIPA server than what the RFCs allow. From this PoV, it is more important for idmsvc to pass registration even with slightly invalid DNS label than being valid but failing. Idmsvc doesn't use the value for DNS operations. Signed-off-by: Petr Vobornik <[email protected]>
Location name should be a DNS label and could be matched by a regex. Correct one could be "^[a-zA-Z0-9]([a-zA-Z0-9\\-]*[a-zA-Z0-9])*$". PR podengo-project#21 has a discussion about it. But users can defined more values in IPA location in the FreeIPA server than what the RFCs allow. From this PoV, it is more important for idmsvc to pass registration even with slightly invalid DNS label than being valid but failing. Idmsvc doesn't use the value for DNS operations. Signed-off-by: Petr Vobornik <[email protected]>
Location name should be a DNS label and could be matched by a regex. Correct one could be "^[a-zA-Z0-9]([a-zA-Z0-9\\-]*[a-zA-Z0-9])*$". PR #21 has a discussion about it. But users can defined more values in IPA location in the FreeIPA server than what the RFCs allow. From this PoV, it is more important for idmsvc to pass registration even with slightly invalid DNS label than being valid but failing. Idmsvc doesn't use the value for DNS operations. Signed-off-by: Petr Vobornik <[email protected]>
As DNS labels are case insensitive and RFC 1035 allows upper case.
Before this change the regex was more restricitive than what was possible to define in IPA location.