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

support ip in san #553

Closed
wants to merge 1 commit into from
Closed

support ip in san #553

wants to merge 1 commit into from

Conversation

gattjoe
Copy link

@gattjoe gattjoe commented Feb 8, 2022

Addresses 544

Adds the IPv4Address Type and controls for it when generating output.

@nabla-c0d3
Copy link
Owner

nabla-c0d3 commented Mar 13, 2022

Hello and thanks for looking into this.

I think we'll need to treat IP addresses separately from DNS names, similarly to the suggested output in #544 :

   DNS Subject Alternative Names:     ['server', 'server.domain']
   IP Subject Alternative Names:      ['192.168.1.10']     <- This is what I would like to see

Otherwise there's a risk of "confusion" in the console output and when performing the actual hostname validation.

The case where we're validating using an IP should be more explicit; something like (in _certificate_matches_hostname()):

    subj_alt_names = tuple([("DNS", name) for name in extract_dns_subject_alternative_names(certificate)])
    subj_alt_names.extend(tuple([("IP Address", ip_addr) for ip_addrin extract_ip_address_subject_alternative_names(certificate)]))
    
    certificate_names = {
        "subject": (tuple([("commonName", name) for name in get_common_names(cert_subject)]),),
        "subjectAltName": subj_alt_names 
    }
```

Having a small test for this would be useful too. Thanks!

@nabla-c0d3
Copy link
Owner

This was implemented in a slightly different way - see #544

@nabla-c0d3 nabla-c0d3 closed this Nov 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants