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

Clarify flags argument of X509_check_ip #17536

Closed
wants to merge 1 commit into from

Conversation

tniessen
Copy link
Contributor

Because no supported flag affects the behavior of X509_check_ip, the flags argument currently has no effect. Clarify this in the documentation to avoid confusion.

Checklist
  • documentation is added or updated

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jan 17, 2022
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Jan 17, 2022
tniessen added a commit to tniessen/node that referenced this pull request Jan 17, 2022
None of the supported options have any effect on X509_check_ip_asc.

(iPAddress is not a typo, it is what RFC 5280 calls subject alternative
names that are IP addresses.)

Refs: openssl/openssl#17536
tniessen added a commit to tniessen/node that referenced this pull request Jan 17, 2022
None of the supported options have any effect on X509_check_ip_asc.

Refs: openssl/openssl#17536
@paulidale
Copy link
Contributor

X509_check_ip passes the flags to do_x509_check which does use them.

@tniessen
Copy link
Contributor Author

X509_check_ip passes the flags to do_x509_check which does use them.

@paulidale While that's technically true, I don't think any of the flags affect the behavior of do_x509_check when called in the context of X509_check_ip. And that's exactly the confusion that this change is trying to address :)


X509_CHECK_FLAG_ALWAYS_CHECK_SUBJECT has no effect since X509_check_ip never considers the certificate subject, it only considers subject alternative names. Similarly, X509_CHECK_FLAG_NEVER_CHECK_SUBJECT has no effect since it matches the default behavior of X509_check_ip.

X509_check_ip sets check_type to GEN_IPADD and thus cnid to NID_undef in do_x509_check. Because cnid == NID_undef, the flags X509_CHECK_FLAG_ALWAYS_CHECK_SUBJECT and X509_CHECK_FLAG_NEVER_CHECK_SUBJECT have no effect.

if (san_present && !(flags & X509_CHECK_FLAG_ALWAYS_CHECK_SUBJECT))
return 0;
}
/* We're done if CN-ID is not pertinent */
if (cnid == NID_undef || (flags & X509_CHECK_FLAG_NEVER_CHECK_SUBJECT))
return 0;


The remaining four flags (X509_CHECK_FLAG_NO_WILDCARD, X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS, X509_CHECK_FLAG_MULTI_LABEL_WILDCARDS, X509_CHECK_FLAG_SINGLE_LABEL_SUBDOMAINS) only affect equal_wildcard, which is only used by X509_check_host. For these flags, that is already documented:

If set, B<X509_CHECK_FLAG_NO_WILDCARDS> disables wildcard
expansion; this only applies to B<X509_check_host>.
If set, B<X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS> suppresses support
for "*" as wildcard pattern in labels that have a prefix or suffix,
such as: "www*" or "*www"; this only applies to B<X509_check_host>.
If set, B<X509_CHECK_FLAG_MULTI_LABEL_WILDCARDS> allows a "*" that
constitutes the complete label of a DNS name (e.g. "*.example.com")
to match more than one label in B<name>; this flag only applies
to B<X509_check_host>.
If set, B<X509_CHECK_FLAG_SINGLE_LABEL_SUBDOMAINS> restricts B<name>
values which start with ".", that would otherwise match any sub-domain
in the peer certificate, to only match direct child sub-domains.
Thus, for instance, with this flag set a B<name> of ".example.com"
would match a peer certificate with a DNS name of "www.example.com",
but would not match a peer certificate with a DNS name of
"www.sub.example.com"; this flag only applies to B<X509_check_host>.

doc/man3/X509_check_host.pod Outdated Show resolved Hide resolved
@t8m t8m added branch: 3.0 Merge to openssl-3.0 branch branch: master Merge to master branch triaged: documentation The issue/pr deals with documentation (errors) labels Jan 18, 2022
Because no supported flag affects the behavior of X509_check_ip, the
flags argument currently has no effect.
@paulidale
Copy link
Contributor

equal_case and equal_nocase call through to skip_prefix which uses _X509_CHECK_FLAG_DOT_SUBDOMAINS and X509_CHECK_FLAG_SINGLE_LABEL_SUBDOMAINS. This indicates to me that the documentation is wrong :(

Sigh, this is way too convoluted.

@tniessen
Copy link
Contributor Author

equal_case and equal_nocase call through to skip_prefix which uses _X509_CHECK_FLAG_DOT_SUBDOMAINS and X509_CHECK_FLAG_SINGLE_LABEL_SUBDOMAINS. This indicates to me that the documentation is wrong :(

@paulidale I'm not sure I understand what this means. IP addresses are not stored as text (RFC 5280):

   When the subjectAltName extension contains an iPAddress, the address
   MUST be stored in the octet string in "network byte order", as
   specified in [RFC791].  The least significant bit (LSB) of each octet
   is the LSB of the corresponding byte in the network address.  For IP
   version 4, as specified in [RFC791], the octet string MUST contain
   exactly four octets.  For IP version 6, as specified in
   [RFC2460], the octet string MUST contain exactly sixteen octets.

As far as I can tell, X509_check_ip should only match if the given IP address is equal to the stored IP address. No string processing of any kind should be performed.

@paulidale
Copy link
Contributor

X509_check_ip calls do_x509_check with a type of GEN_IPADD. This sets equal to equal_case. When called this calls skip_prefix where the flags are used. However, it looks like removing _X509_CHECK_FLAG_DOT_SUBDOMAINS makes this behave as desired.

Way too convoluted.

@paulidale paulidale added the approval: review pending This pull request needs review by a committer label Jan 19, 2022
@tniessen
Copy link
Contributor Author

Way too convoluted.

Agreed 😃

@t8m t8m added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jan 19, 2022
tniessen added a commit to tniessen/node that referenced this pull request Jan 19, 2022
None of the supported options have any effect on X509_check_ip_asc.

Refs: openssl/openssl#17536
tniessen added a commit to nodejs/node that referenced this pull request Jan 19, 2022
None of the supported options have any effect on X509_check_ip_asc.

Refs: openssl/openssl#17536

PR-URL: #41571
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
tniessen added a commit to tniessen/node that referenced this pull request Jan 19, 2022
Wildcard options do not affect X509_check_email.

Refs: openssl/openssl#17536
Refs: nodejs#41571
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jan 20, 2022
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request Jan 20, 2022
Because no supported flag affects the behavior of X509_check_ip, the
flags argument currently has no effect.

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #17536)

(cherry picked from commit 2d280fe)
openssl-machine pushed a commit that referenced this pull request Jan 20, 2022
Because no supported flag affects the behavior of X509_check_ip, the
flags argument currently has no effect.

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #17536)
@t8m
Copy link
Member

t8m commented Jan 20, 2022

Merged to master and 3.0 branches. Thank you for your contribution.

@t8m t8m closed this Jan 20, 2022
panva pushed a commit to nodejs/node that referenced this pull request Jan 22, 2022
Wildcard options do not affect X509_check_email.

Refs: openssl/openssl#17536
Refs: #41571

PR-URL: #41599
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
BethGriggs pushed a commit to nodejs/node that referenced this pull request Jan 25, 2022
None of the supported options have any effect on X509_check_ip_asc.

Refs: openssl/openssl#17536

PR-URL: #41571
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BethGriggs pushed a commit to nodejs/node that referenced this pull request Jan 25, 2022
Wildcard options do not affect X509_check_email.

Refs: openssl/openssl#17536
Refs: #41571

PR-URL: #41599
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
None of the supported options have any effect on X509_check_ip_asc.

Refs: openssl/openssl#17536

PR-URL: nodejs#41571
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
Wildcard options do not affect X509_check_email.

Refs: openssl/openssl#17536
Refs: nodejs#41571

PR-URL: nodejs#41599
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
danielleadams pushed a commit to nodejs/node that referenced this pull request Feb 28, 2022
None of the supported options have any effect on X509_check_ip_asc.

Refs: openssl/openssl#17536

PR-URL: #41571
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit to nodejs/node that referenced this pull request Feb 28, 2022
Wildcard options do not affect X509_check_email.

Refs: openssl/openssl#17536
Refs: #41571

PR-URL: #41599
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
danielleadams pushed a commit to nodejs/node that referenced this pull request Mar 2, 2022
None of the supported options have any effect on X509_check_ip_asc.

Refs: openssl/openssl#17536

PR-URL: #41571
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit to nodejs/node that referenced this pull request Mar 2, 2022
Wildcard options do not affect X509_check_email.

Refs: openssl/openssl#17536
Refs: #41571

PR-URL: #41599
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
danielleadams pushed a commit to nodejs/node that referenced this pull request Mar 3, 2022
None of the supported options have any effect on X509_check_ip_asc.

Refs: openssl/openssl#17536

PR-URL: #41571
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit to nodejs/node that referenced this pull request Mar 3, 2022
Wildcard options do not affect X509_check_email.

Refs: openssl/openssl#17536
Refs: #41571

PR-URL: #41599
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
danielleadams pushed a commit to nodejs/node that referenced this pull request Mar 14, 2022
None of the supported options have any effect on X509_check_ip_asc.

Refs: openssl/openssl#17536

PR-URL: #41571
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit to nodejs/node that referenced this pull request Mar 14, 2022
Wildcard options do not affect X509_check_email.

Refs: openssl/openssl#17536
Refs: #41571

PR-URL: #41599
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch triaged: documentation The issue/pr deals with documentation (errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants