Skip to content

Conversation

alex
Copy link
Member

@alex alex commented Jun 28, 2015

No description provided.

@alex
Copy link
Member Author

alex commented Jun 28, 2015

This is obviously pretty incomplete, but also I don't understand why it's failing the way it is.

@alex alex added this to the Tenth Release milestone Jun 28, 2015
@codecov-io
Copy link

Current coverage is 99.85%

Merging #2085 into master will change coverage by +1.18% by fce90b9

Coverage Diff

@@            master   #2085   diff @@
======================================
  Files          109     109       
  Stmts        10766   10803    +37
  Branches      1233    1238     +5
  Methods          0       0       
======================================
+ Hit          10623   10787   +164
+ Partial         19      16     -3
+ Missed         124       0   -124

Powered by Codecov

@reaperhulk
Copy link
Member

This LGTM right now with the TODOs (GC on GENERAL_NAMES and IDNA support) still required. I assume we're planning to land support for each general name type as a separate PR? So after this DNS PR we'll need ones for URI, RegisteredID, IP, dirname, and email.

@alex
Copy link
Member Author

alex commented Jul 3, 2015

That's a good question, do you have any sense of how much code it'll be for
each of those GN types?

On Fri, Jul 3, 2015 at 11:35 AM, Paul Kehrer notifications@github.com
wrote:

This LGTM right now with the TODOs (GC on GENERAL_NAMES and IDNA support)
still required. I assume we're planning to land support for each general
name type as a separate PR? So after this DNS PR we'll need ones for URI,
RegisteredID, IP, dirname, and email.


Reply to this email directly or view it on GitHub
#2085 (comment).

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: 125F 5C67 DFE9 4084

@reaperhulk
Copy link
Member

RegisteredID will be simple and IP probably will be as well (at least for now since there's no need to handle the netmask stuff in #2095). URI requires IDNA handling again, as does email. dirname requires encoding x509 names, but we have support for that already so that should be reasonably simple.

@kevgliss
Copy link

kevgliss commented Jul 3, 2015

+1 For a merge when you guys are ready, I've got a branch waiting for testing on some sweet CSR SAN support :-).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handle wildcards here or in a followup PR? Leading periods as well, but that's not hugely important until we support NameConstraints construction I suppose.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do wildcards now.

On Sun, Jul 5, 2015 at 12:23 PM, Paul Kehrer notifications@github.com
wrote:

In src/cryptography/hazmat/backends/openssl/backend.py
#2085 (comment):

@@ -136,6 +138,39 @@ def _encode_basic_constraints(backend, basic_constraints):
return pp, r

+def _encode_subject_alt_name(backend, san):

  • general_names = backend._lib.GENERAL_NAMES_new()
  • assert general_names != backend._ffi.NULL
  • general_names = backend._ffi.gc(
  •    general_names, backend._lib.GENERAL_NAMES_free
    
  • )
  • for alt_name in san:
  •    gn = backend._lib.GENERAL_NAME_new()
    
  •    assert gn != backend._ffi.NULL
    
  •    if isinstance(alt_name, x509.DNSName):
    
  •        gn.type = backend._lib.GEN_DNS
    
  •        ia5 = backend._lib.ASN1_IA5STRING_new()
    
  •        assert ia5 != backend._ffi.NULL
    
  •        value = idna.encode(alt_name.value)
    

Handle wildcards here or in a followup PR? Leading periods as well, but
that's not hugely important until we support NameConstraints construction I
suppose.


Reply to this email directly or view it on GitHub
https://github.com/pyca/cryptography/pull/2085/files#r33896147.

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: 125F 5C67 DFE9 4084

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will leak if the general name is not a DNSName right now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ffff. thanks

On Sun, Jul 5, 2015 at 12:32 PM, Paul Kehrer notifications@github.com
wrote:

In src/cryptography/hazmat/backends/openssl/backend.py
#2085 (comment):

@@ -136,6 +138,44 @@ def _encode_basic_constraints(backend, basic_constraints):
return pp, r

+def _encode_subject_alt_name(backend, san):

  • general_names = backend._lib.GENERAL_NAMES_new()
  • assert general_names != backend._ffi.NULL
  • general_names = backend._ffi.gc(
  •    general_names, backend._lib.GENERAL_NAMES_free
    
  • )
  • for alt_name in san:
  •    gn = backend._lib.GENERAL_NAME_new()
    

This will leak if the general name is not a DNSName right now.


Reply to this email directly or view it on GitHub
https://github.com/pyca/cryptography/pull/2085/files#r33896254.

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: 125F 5C67 DFE9 4084

@alex
Copy link
Member Author

alex commented Jul 5, 2015

jenkins, retest this please

reaperhulk added a commit that referenced this pull request Jul 5, 2015
Initial code to encode SANs
@reaperhulk reaperhulk merged commit 5b57cbf into pyca:master Jul 5, 2015
@alex alex deleted the encode-san branch July 5, 2015 19:08
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

4 participants