[GitHub] ant pull request #64: Add support for SAN extension in GenerateKey task

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] ant pull request #64: Add support for SAN extension in GenerateKey task

jaikiran
GitHub user jnsnkrllive opened a pull request:

    https://github.com/apache/ant/pull/64

    Add support for SAN extension in GenerateKey task

    Beginning with Java 7, keytool supports the SubjectAlternativeName extension on X.509 certificates.
    [Java 7 Keytool Documentation](https://docs.oracle.com/javase/7/docs/technotes/tools/windows/keytool.html)
   
    This enhancement will be useful to users who use Ant to issue certificates for their project. Especially, for example, if the project is a web application that is expected to be accessed through modern browsers who are beginning to drop support for Common Name.
    [Chrome 58 Deprecations - Remove support for commonName matching in certificates](https://developers.google.com/web/updates/2017/03/chrome-58-deprecations#remove_support_for_commonname_matching_in_certificates)
   
    Targeting 1.9.x branch now, this can be merged into master too.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/jnsnkrllive/ant 1.9.x

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/ant/pull/64.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #64
   
----
commit e291c451995d1f89a2d22e0b4f71058ef453e1bc
Author: Karl Jansen <jnsnkrl@...>
Date:   2018-07-11T21:19:29Z

    Add support for SAN extension in GenerateKey task

----


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant issue #64: Add support for SAN extension in GenerateKey task

jaikiran
Github user asfgit commented on the issue:

    https://github.com/apache/ant/pull/64
 
    Can one of the admins verify this patch?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant issue #64: Add support for SAN extension in GenerateKey task

jaikiran
In reply to this post by jaikiran
Github user asfgit commented on the issue:

    https://github.com/apache/ant/pull/64
 
    Can one of the admins verify this patch?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant issue #64: Add support for SAN extension in GenerateKey task

jaikiran
In reply to this post by jaikiran
Github user jaikiran commented on the issue:

    https://github.com/apache/ant/pull/64
 
    this is ok to test


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant issue #64: Add support for SAN extension in GenerateKey task

jaikiran
In reply to this post by jaikiran
Github user asfgit commented on the issue:

    https://github.com/apache/ant/pull/64
 
   
    Refer to this link for build results (access rights to CI server needed):
    https://builds.apache.org/job/Ant%20Github-PR-Windows/70/



---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant pull request #64: Add support for SAN extension in GenerateKey task

jaikiran
In reply to this post by jaikiran
Github user jaikiran commented on a diff in the pull request:

    https://github.com/apache/ant/pull/64#discussion_r202054728
 
    --- Diff: src/main/org/apache/tools/ant/taskdefs/GenerateKey.java ---
    @@ -413,6 +429,16 @@ public void execute() throws BuildException {
                 sb.append("\" ");
             }
     
    +        if (useExtension) {
    +            sb.append("-ext ");
    --- End diff --
   
    Should this be appended only if `saname` is not null? I haven't given it a try but does the keytool work fine if we end up passing "-ext" without any extension name value?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant issue #64: Add support for SAN extension in GenerateKey task

jaikiran
In reply to this post by jaikiran
Github user asfgit commented on the issue:

    https://github.com/apache/ant/pull/64
 
   
    Refer to this link for build results (access rights to CI server needed):
    https://builds.apache.org/job/Ant%20Github-PR-Linux/64/



---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant pull request #64: Add support for SAN extension in GenerateKey task

jaikiran
In reply to this post by jaikiran
Github user jnsnkrllive commented on a diff in the pull request:

    https://github.com/apache/ant/pull/64#discussion_r202096699
 
    --- Diff: src/main/org/apache/tools/ant/taskdefs/GenerateKey.java ---
    @@ -413,6 +429,16 @@ public void execute() throws BuildException {
                 sb.append("\" ");
             }
     
    +        if (useExtension) {
    +            sb.append("-ext ");
    --- End diff --
   
    Good question. I did some testing and here's what I found:
   
    keytool would fail if we pass "-ext" without a name.
    `keytool -genkey -alias "keystorename" -keystore "keystorename" -storepass "secret" -keypass "secret" -ext`
    > Command option -ext needs an argument.
   
    However, we won't ever append "-ext" without also appending a name too. Currently the only way to append "-ext" is when useExtension is true, which only happens if the sname attribute is included in the definition AND the java version is 1.7 or higher.
   
    keytool works fine if the saname attribute is not included in the definition. "useExtension" would be false (because "setSaname" would never get called) and it'd skip over the code block beginning on line 432.
   
    However, keytool throws an exception if saname="" is used in the definition
    `[genkey] keytool error: java.lang.Exception: Illegal item in san=`
    This definition of the task doesn't meet the requirements specified by keytool. Should ant handle this differently or defer to keytool for handing the invalid use? It doesn't look like we are doing any special validation on the other arguments (e.g. "sigalg" which is just a string in this Task but keytool only accepts certain values for that string).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant pull request #64: Add support for SAN extension in GenerateKey task

jaikiran
In reply to this post by jaikiran
Github user jaikiran commented on a diff in the pull request:

    https://github.com/apache/ant/pull/64#discussion_r202287386
 
    --- Diff: src/main/org/apache/tools/ant/taskdefs/GenerateKey.java ---
    @@ -413,6 +429,16 @@ public void execute() throws BuildException {
                 sb.append("\" ");
             }
     
    +        if (useExtension) {
    +            sb.append("-ext ");
    --- End diff --
   
    >> However, we won't ever append "-ext" without also appending a name too. Currently the only way to append "-ext" is when useExtension is true, which only happens if the sname attribute is included in the definition AND the java version is 1.7 or higher.
   
    Hi @jnsnkrllive, The reason I brought it up is because I see that the `san` extension name gets added only if the `saname` is set to non-null. Whereas the `ext` argument gets passed whether or not `saname` is null because the `useExtension` gets set to `true` irrespective of whether or not the saname is null (imagine someone calling setSaname with a null value). Perhaps, we don't need "useExtension" field for now (until we introduce more supported extension names)? That way you can add the "-ext -san=<value>" if there's non-null `saname` set?
   
   



---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant pull request #64: Add support for SAN extension in GenerateKey task

jaikiran
In reply to this post by jaikiran
Github user jnsnkrllive commented on a diff in the pull request:

    https://github.com/apache/ant/pull/64#discussion_r202364682
 
    --- Diff: src/main/org/apache/tools/ant/taskdefs/GenerateKey.java ---
    @@ -413,6 +429,16 @@ public void execute() throws BuildException {
                 sb.append("\" ");
             }
     
    +        if (useExtension) {
    +            sb.append("-ext ");
    --- End diff --
   
    Hey @jaikiran, thanks for pointing that out. I agree, `useExtension` isn't necessary right now since only 1 extension is being supported. I'll fix this now.
    This mechanism or something similar/better can be introduced when we add support for another extension sometime in the future, when it's actually needed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant issue #64: Add support for SAN extension in GenerateKey task

jaikiran
In reply to this post by jaikiran
Github user asfgit commented on the issue:

    https://github.com/apache/ant/pull/64
 
   
    Refer to this link for build results (access rights to CI server needed):
    https://builds.apache.org/job/Ant%20Github-PR-Linux/65/



---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant issue #64: Add support for SAN extension in GenerateKey task

jaikiran
In reply to this post by jaikiran
Github user asfgit commented on the issue:

    https://github.com/apache/ant/pull/64
 
   
    Refer to this link for build results (access rights to CI server needed):
    https://builds.apache.org/job/Ant%20Github-PR-Windows/71/



---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant issue #64: Add support for SAN extension in GenerateKey task

jaikiran
In reply to this post by jaikiran
Github user jaikiran commented on the issue:

    https://github.com/apache/ant/pull/64
 
    Thank you @jnsnkrllive. This now looks fine to me. I'll merge this later tonight/tomorrow, once our currently in-progress Ant release(s) are officially announced.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant pull request #64: Add support for SAN extension in GenerateKey task

jaikiran
In reply to this post by jaikiran
Github user asfgit closed the pull request at:

    https://github.com/apache/ant/pull/64


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant issue #64: Add support for SAN extension in GenerateKey task

jaikiran
In reply to this post by jaikiran
Github user jaikiran commented on the issue:

    https://github.com/apache/ant/pull/64
 
    @jnsnkrllive I've merged this into 1.9.x as well as master branches. Thank you for the PR. I've also included your name "Karl Jansen" in our contributors list. If you like to use a different name, let us know.
   
    Finally, would you be interested in the updating the manual of the generatekey task to make a mention of this new attribute and send a new PR? If not, I'll get to it later in the week myself.



---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]