<html>
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <p>Hi Anthony!</p>
    <p><br>
    </p>
    <p>A few minor comments:</p>
    <p><b>AlgorithmChecker.java</b></p>
    <p>It would be more consistent to use {@code ...} tags in place of
      <code>...</code></p>
    <p><br>
    </p>
    <p><b>DisabledAlgorithmConstraints.java</b></p>
    <pre style="color: rgb(0, 0, 0); font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; widows: 1; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(238, 238, 238);"><span class="new" style="color: blue; font-weight: bold;">275                     Matcher dmatch = denyAfterPattern.matcher(entry);</span></pre>
    <pre style="color: rgb(0, 0, 0); font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; widows: 1; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(238, 238, 238);"><span class="new" style="color: blue; font-weight: bold;">296                     } else if (dmatch.matches()) {</span></pre>
    <br>
    It might be a bit more efficient to reuse already declared `Matcher
    matcher` like this:<br>
    <br>
        } else if (matcher.usePattern(denyAfterPattern).matches()) {<br>
    <br>
    <br>
    <br>
    With kind regards,<br>
    Ivan<br>
    <br>
    <br>
    <div class="moz-cite-prefix">On 13.05.2016 0:34, Anthony Scarpino
      wrote:<br>
    </div>
    <blockquote cite="mid:5734F6F9.1090501@oracle.com" type="cite">I've
      updated the webrev to at:
      <br>
        <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~ascarpino/8154005/webrev.01/">http://cr.openjdk.java.net/~ascarpino/8154005/webrev.01/</a>
      <br>
      <br>
      Comments addressed below...
      <br>
      <br>
      On 05/11/2016 04:55 PM, <a class="moz-txt-link-abbreviated" href="mailto:ecki@zusammenkunft.net">ecki@zusammenkunft.net</a> wrote:
      <br>
      <blockquote type="cite">Hello,
        <br>
        <br>
        In AlgorithmChecker the Javadoc seems to not follow "@param name
        <br>
        desc" format (in two places). Also it should most likely
        describe
        <br>
        something like "time the signature claimed to be made to check
        time
        <br>
        range limited ciphers after that date or similiar)
        <br>
        <br>
        * @param PKIXParameter timestamp (or null)
        <br>
      </blockquote>
      <br>
      Thanks for seeing that.. I updated them.
      <br>
      <br>
      <blockquote type="cite">
        <br>
        DisabledAlgorithmConstrained: The regular expression allows
        <br>
        denyafter20160101 its clear, but \s+ might be clearer? Can
        optional
        <br>
        iso  Idate seperators,  be added. "(\d {4})-?(\d {2})-?...."
        <br>
      </blockquote>
      <br>
      I think I prefer to require '-' as standard syntax and not use
      YYYYMMDD.  Sometimes YYYYMMDD not as clear to read as YYYY-MM-DD.
      I would like to not have two valid formats.
      <br>
      <br>
      <blockquote type="cite">
        <br>
        The lowercase constraint classes are rather strange, but fits
        into
        <br>
        existing code...
        <br>
        <br>
        I dont see in the patch how the date param is certified. Is this
        only
        <br>
        the issued date as certified (by the weak) signature or does it
        look
        <br>
        at timestamps (especially codesigning) too?
        <br>
      </blockquote>
      <br>
      The date is passed as part of PKIX, it's optional that PKIX can
      have a date parameter set to specify a time date.
      <br>
      The date disallows a certificate with the disabled algorithm on
      that date.  It does not check validity of the certificate.  This
      is meant to shutoff the algorithm in certificate checking.  There
      maybe a exception to this to allowing codesigning certificates a
      bit longer, but that hasn't been completely decided yet.
      <br>
      <br>
      <blockquote type="cite">
        <br>
        There are a few conditions which could be unit tested:
        <br>
        <br>
        RSA keySize <= 1024 & disablesAfter 20160101 SHA1
        disabledAfter
        <br>
        20160102 // valid RSA disabledAfter 20160101 & disabledAfter
        20160101
        <br>
        // not valid Etc
        <br>
      </blockquote>
      <br>
      Yes.  There are a number of tests that are in the closed test repo
      because they use certificates that are not for public use.
      <br>
      <br>
      thanks
      <br>
      <br>
      Tony
      <br>
      <br>
      <blockquote type="cite">
        <br>
        Gruss
        <br>
      </blockquote>
      >
      <br>
      <blockquote type="cite">Bernd
        <br>
        <br>
      </blockquote>
      <br>
      <br>
    </blockquote>
    <br>
  </body>
</html>