add a method to collect the DNS names from a certificate

[Geal]

Fix #64

I am not too happy about it yet, I'd like to remove the ToString implementation for DNSName, since there's a Into<&str> for DNSNameRef.
Unfortunately, I have not been able to make the function return a vector of DNSNameRefyet.
For reference, the code I'm currently trying to build is:

#[cfg(feature = "std")]
pub fn list_cert_dns_names<'a>(cert: &'e super::EndEntityCert<'a>)
                                   -> Result<Vec<DNSNameRef<'a>>, Error> {
    let cert = &cert.inner;
    let names = std::cell::RefCell::new(Vec::new());

    iterate_names(cert.subject, cert.subject_alt_name, Ok(()), &|name| {
        match name {
            GeneralName::DNSName(presented_id) => {
                names.borrow_mut().push(DNSNameRef(presented_id));
            },
            _ => ()
        }
        NameIteration::KeepGoing
    }).map(|_| names.into_inner())
}

But there's a conflict between the lifetime of name (created in iterate_names and passed to the closure) and the lifetime of the DNSNameRef returned as result.

[briansmith]

Looks pretty good.

In addition to this test, we should also have the following test edge cases:

1. There are no DNS names in the cert, but the subject common name looks like a DNS name ("CN=example.com"). This should return an empty result.
2. There are multiple subjectAltNames, and the last one is an invalid DNS name in subjectAltNames. You can create one of these certificates by starting with a valid certificate and modifying it in a hex editor or similar, e.g. replace s/./:/ in "example.com".

[briansmith]

I think this might make sense as Into<String> but otherwise there's already another way of doing this, right? x.as_ref().into().into(); for x: DNSName. So I think we should avoid adding this.

In particular, one of my overall goals is to encourage people to use stronger types than String and str for values and so I try to minimize the convenience features for getting those types from stronger types.

[Geal]

I removed it

[briansmith]

Why not let mut names = Vec::new()? I don't think we should need RefCell here.

[Geal]

I thought so too, but I get the following error when I do it: error[E0387]: cannot borrow data mutably in a captured outer variable in an Fn closure

[briansmith]

NIT:

You can write this as:

let expected_names = {
    const EXPECTED_NAMES: &'static [&'static str] = &[
        "account.netflix.com",
        ....
       "www.netflix.com",
    ];
    HashSet::from_iter(EXPECTED_NAMES.iter())
};

[Geal]

done

[briansmith]

I think this should be called dns_names() instead of list_dns_names().

[Geal]

done

[briansmith]

// Ensure that converting the list to a set doesn't throw away
// any duplicates that aren't supposed to be there
assert_eq!(actual_names.len(), expected_names.len());

let actual_names = HashSet::from_iter(actual_names.iter()
    .map(|dns_name| dns_name.as_ref().into())) // Maybe `into::<&str>` for clarity.

[Geal]

added

[briansmith]

Unfortunately, I have not been able to make the function return a vector of DNSNameRef yet.

I will look into it now.

[Geal]

I'll add more integration tests as you suggested, then it should be good to go :)

[Geal]

the new tests are there, and the build failures are apparently unrelated to this PR (*ring* compilation failures on appveyor, and the beta toolchain not installed for the 2 xcode builds in travis).

[Geal]

hello, friendly ping on this :)
let me know if there's anything to change

[norcalli]

The build failures seem a little off on this. For AppVeyor, I tried compiling his branch on 1.19.0 and got different errors. For travis-ci, the only build failure was on Rust: beta Xcode 7.2, where the error was

error: toolchain 'beta' is not installed
info: caused by: not a directory: '/Users/travis/.rustup/toolchains/beta-x86_64-apple-darwin'

Which seems to be a failure of the build system, not the code, so it seems like this branch should be ready to merge/final review.

[briansmith]

@Geal I don't know why I didn't comment on this here, but please see #66. At a minimum, we should have a test that demonstrates what happens when there is only a wildcard *.example.com and another test thta demonstrates what happens when there are both wildcard and non-wildcard subjectAltNames.

[briansmith]

The ideal state of wildcard support is hinted at in #66: Distinguish wildcard and non-wildcard entries with distinct enum variants of a new type. I would be willing to defer that ideal wildcard support to another PR but we at least need to decide to do when this function encounters a wildcard entry and we need the tests. It's probably less work to solve it all at once properly. WDYT?

[Geal]

What should be expected when there are both wildcard and non wildcard? Can the wildcard match the non wildcard ? For sozu I will use the wildcard entries as well. I can go over the code next week

…

On Fri, Sep 28, 2018, 04:57 Brian Smith ***@***.***> wrote: The ideal state of wildcard support is hinted at in #66 <#66>: Distinguish wildcard and non-wildcard entries with distinct enum variants of a new type. I would be willing to defer that ideal wildcard support to another PR but we at least need to decide to do when this function encounters a wildcard entry and we need the tests. It's probably less work to solve it all at once properly. WDYT? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#65 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHSAHm5h_1E6EEg-QiayyTj6ULEFIuEks5ufZAWgaJpZM4ROtX1> .

[briansmith]

Thanks for contributing this! This seems to have been subsumed by #91 so I'm optimistically closing this in favor of that one.