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

x509.Cert JSON output removes invalid URL names #183

Open
zzma opened this issue Feb 4, 2019 · 4 comments
Open

x509.Cert JSON output removes invalid URL names #183

zzma opened this issue Feb 4, 2019 · 4 comments

Comments

@zzma
Copy link
Contributor

zzma commented Feb 4, 2019

Marshaling output to JSON removes names that are not valid URLs - see isValidName function in x509/json.go. This should either be documented or changed, since it does not reflect the actual names contained in a certificate. Sidenote: this logic takes up a significant amount of processing time (discovered while performance profiling)

@zakird
Copy link
Member

zakird commented Feb 4, 2019

Can you provide a few examples of this?

@zzma
Copy link
Contributor Author

zzma commented Feb 4, 2019

Here's an example I have offhand that removes the names mail.xn----7sba4atictgs.xn--p1ai www.xn----7sba4atictgs.xn--p1ai and www.xn----7sba4atictgs.soft-angel.ru from the cert below. It is surprising to me that it's removing the seemingly valid url www.xn----7sba4atictgs.soft-angel.ru....I can dig into that a little more later.

Let me know if you need more examples.

MIIHGzCCBgOgAwIBAgISA7Do9dT3TYSxgIiIk5DBs5l+MA0GCSqGSIb3DQEBCwUAMEoxCzAJBgNVBAYTAlVTMRYwFAYDVQQKEw1MZXQncyBFbmNyeXB0MSMwIQYDVQQDExpMZXQncyBFbmNyeXB0IEF1dGhvcml0eSBYMzAeFw0xNzA5MDkyMjI5MDBaFw0xNzEyMDgyMjI5MDBaMBgxFjAUBgNVBAMTDWludHByb2R1Y3QucnUwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDS8kZVramG3K8qzuBBEjLtI51SHySBvbebIiqYLde3bDZKyAs4HP5BbOMX4H79QvzDLfQQ53O8N+WSDFd453aK3kxFGrvUrN0zBdImYmUFRmEA06R5pUHgdYw7W+H7eiivPhq/g9nRj6hf8Ofz6AeiAlFwtTFKpDhWxTeagrBkpiepFcD5zQNiBfS8um3f7qOfxq4CWrw6p4oxgrI9/YQDpIlDAtnyKTQNi4GHV+QhDia1zNBqvJUwQ07wYO8bZsusCYKQvd8YFdgnGG2f26hczY6I0vFG2Urog5h8qur/keMx6VqRTnfAIihnRykBOPwRgJBFP2lOCqab3xSHETGJAgMBAAGjggQrMIIEJzAOBgNVHQ8BAf8EBAMCBaAwHQYDVR0lBBYwFAYIKwYBBQUHAwEGCCsGAQUFBwMCMAwGA1UdEwEB/wQCMAAwHQYDVR0OBBYEFCtJ9Q3gpsIa9BlY9qzfJqZjlDhkMB8GA1UdIwQYMBaAFKhKamMEfd265tE5t6ZFZe/zqOyhMG8GCCsGAQUFBwEBBGMwYTAuBggrBgEFBQcwAYYiaHR0cDovL29jc3AuaW50LXgzLmxldHNlbmNyeXB0Lm9yZzAvBggrBgEFBQcwAoYjaHR0cDovL2NlcnQuaW50LXgzLmxldHNlbmNyeXB0Lm9yZy8wggI0BgNVHREEggIrMIICJ4IKYXJtYXJpZS5ydYIVYXJtYXJpZS5zb2Z0LWFuZ2VsLnJ1ghFlY28uc29mdC1hbmdlbC5ydYINaW50cHJvZHVjdC5ydYIYaW50cHJvZHVjdC5zb2Z0LWFuZ2VsLnJ1ghRrYXJ0ZXIuc29mdC1hbmdlbC5ydYIJa2FydGVyLnN1gg9tYWlsLmFybWFyaWUucnWCEm1haWwuaW50cHJvZHVjdC5ydYIObWFpbC5rYXJ0ZXIuc3WCIG1haWwueG4tLS0tN3NiYTRhdGljdGdzLnhuLS1wMWFpghN0ZXN0Ni5zb2Z0LWFuZ2VsLnJ1gg53d3cuYXJtYXJpZS5ydYIZd3d3LmFybWFyaWUuc29mdC1hbmdlbC5ydYIVd3d3LmVjby5zb2Z0LWFuZ2VsLnJ1ghF3d3cuaW50cHJvZHVjdC5ydYIcd3d3LmludHByb2R1Y3Quc29mdC1hbmdlbC5ydYIYd3d3LmthcnRlci5zb2Z0LWFuZ2VsLnJ1gg13d3cua2FydGVyLnN1ghd3d3cudGVzdDYuc29mdC1hbmdlbC5ydYIkd3d3LnhuLS0tLTdzYmE0YXRpY3Rncy5zb2Z0LWFuZ2VsLnJ1gh93d3cueG4tLS0tN3NiYTRhdGljdGdzLnhuLS1wMWFpgiB4bi0tLS03c2JhNGF0aWN0Z3Muc29mdC1hbmdlbC5ydYIbeG4tLS0tN3NiYTRhdGljdGdzLnhuLS1wMWFpMIH+BgNVHSAEgfYwgfMwCAYGZ4EMAQIBMIHmBgsrBgEEAYLfEwEBATCB1jAmBggrBgEFBQcCARYaaHR0cDovL2Nwcy5sZXRzZW5jcnlwdC5vcmcwgasGCCsGAQUFBwICMIGeDIGbVGhpcyBDZXJ0aWZpY2F0ZSBtYXkgb25seSBiZSByZWxpZWQgdXBvbiBieSBSZWx5aW5nIFBhcnRpZXMgYW5kIG9ubHkgaW4gYWNjb3JkYW5jZSB3aXRoIHRoZSBDZXJ0aWZpY2F0ZSBQb2xpY3kgZm91bmQgYXQgaHR0cHM6Ly9sZXRzZW5jcnlwdC5vcmcvcmVwb3NpdG9yeS8wDQYJKoZIhvcNAQELBQADggEBAIzwkyuD49y0FpJKptH9YqCYj1DJniGckxRbAxL1ur70sFwjRlvqYo694dm+5Hw1XteWSR3ZGaDMZxjuptdmaBaSdk8aHCUoSkG85THJfRSCCnPxtGQaTifw3lqptdnOQh9o691s3PPzjU6p8KyE/yZmQJPQAE8PnhuTl1Z/E62Zfo3S8XyMADH1Cfxn1ybnJVB+5/scWMKkF9AoNwzUjffWrd7crxIWebkuaySO1OnSW7EZHFru5o5HmN8ChVbiiFgGDFs5tGjGgZu7Q5BNKSITfWGLuFkXZnq2s5lU68MpcE5x/yUxqdszUkJxiyxwcxqwecq0Qs1C3qWymp95pNk=

@justinbastress
Copy link
Contributor

justinbastress commented Feb 4, 2019

The cert seems to be here: https://censys.io/certificates/d859b40f61cb7683459ba101bae740a46a87f4f67e12363b69acb47628f76384/raw

I see that e.g. "mail.xn----7sba4atictgs.xn--p1ai" doesn't show up in "names", but it does show up in "dns_names", which according to the source (see x509/json.go) is what it's trying to do; we end up calling out to govalidator.IsURL() before adding them to the combined list.

There may be a problem with using govalidator.IsURL on dns_names, since they aren't supposed to be URL, but I don't know if that's what you mean here; there are certainly going to be names in the subject / SAN that don't get included in the combined "names" field (for instance, any OtherNames are ignored completely), and that is expected -- they aren't "removed", but rather just not copied to the combined field.

@zzma
Copy link
Contributor Author

zzma commented Feb 4, 2019

That makes sense - when initially using this field I neglected to realize that "names" is not a simple aggregation of the different name fields. I suspect this is not uncommon and I would suggest changing the "names" field to "validated_names" or something that more closely represents what the field contains.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants