RFR 8176350: Usage constraints don't take effect when using PKIX

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR 8176350: Usage constraints don't take effect when using PKIX

Anthony Scarpino
Hi,

I need a code review of this small change.. The PKIX path for usage
checking didn't pass the variant to the checkers because of a previous
needed check for plugins.

http://cr.openjdk.java.net/~ascarpino/8176350/webrev/

thanks

Tony
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8176350: Usage constraints don't take effect when using PKIX

Xuelei Fan-2
Looks fine to me except a minor comment.

In the update of DisabledAlgorithmConstraints.java, the dumping of stack
trace for every checking could increase the debug log size a lot.  There
is no verbose option for 'certpath' debug.  What do you think if only
dumping the log when the usage is not allowed?

Xuelei

On 3/8/2017 1:15 PM, Anthony Scarpino wrote:

> Hi,
>
> I need a code review of this small change.. The PKIX path for usage
> checking didn't pass the variant to the checkers because of a previous
> needed check for plugins.
>
> http://cr.openjdk.java.net/~ascarpino/8176350/webrev/
>
> thanks
>
> Tony
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8176350: Usage constraints don't take effect when using PKIX

Sean Mullan
In reply to this post by Anthony Scarpino
- DisabledAlgorithmConstraints.java

You can just call Thread.dumpStack (which dumps to stderr) here if debug
is set. OTOH, I am concerned this will quickly fill up the log file with
stack traces (which can be long). Do we really need this?

--Sean

On 3/8/17 4:15 PM, Anthony Scarpino wrote:

> Hi,
>
> I need a code review of this small change.. The PKIX path for usage
> checking didn't pass the variant to the checkers because of a previous
> needed check for plugins.
>
> http://cr.openjdk.java.net/~ascarpino/8176350/webrev/
>
> thanks
>
> Tony
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8176350: Usage constraints don't take effect when using PKIX

Anthony Scarpino
In reply to this post by Xuelei Fan-2
Since Sean and you are asking similar questions, I'll answer it here.
It was to be consistent with by using the Debug class.  If a change is
made to Debug to print elsewhere, what I have now is compatible.
Sending it to stderr is basically saying that Debug is a useless class
and we should just print System.err.println() instead.

As for why the stack trace is there, like I said in the comment, if
there is a situation that should or should not have been blocked, we
know what path it took.  There are many methods that send data through
this code and having this debug info allows us to quickly find why it
happened instead of trying to recreating the situation.  Other
constraints deny or allow all ops.  This being usage-based, where SHA1
could be blocked for one usage, but not for another on the same security
property need more detailed debugging, in my opinion.

About the extra data, I don't believe it's that much more and there
isn't a way to find out the information that would be needed.  The
situation of the problem may not be reproducible in our environment
because the user doesn't want to give up their certs or know their app
is performing that usage.   If dumpStack() is less data than
printStackTrace(), I'm fine with switching as long as I get the same info.

It would be nice if there was a verbose option.  We could turn off
AdaptiveX509Selector that prints every check in byte level detail which
overflows jtreg's buffer all the time :-)

Tony

On 03/08/2017 01:42 PM, Xuelei Fan wrote:

> Looks fine to me except a minor comment.
>
> In the update of DisabledAlgorithmConstraints.java, the dumping of stack
> trace for every checking could increase the debug log size a lot.  There
> is no verbose option for 'certpath' debug.  What do you think if only
> dumping the log when the usage is not allowed?
>
> Xuelei
>
> On 3/8/2017 1:15 PM, Anthony Scarpino wrote:
>> Hi,
>>
>> I need a code review of this small change.. The PKIX path for usage
>> checking didn't pass the variant to the checkers because of a previous
>> needed check for plugins.
>>
>> http://cr.openjdk.java.net/~ascarpino/8176350/webrev/
>>
>> thanks
>>
>> Tony

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8176350: Usage constraints don't take effect when using PKIX

Sean Mullan
On 3/8/17 5:52 PM, Anthony Scarpino wrote:
> Since Sean and you are asking similar questions, I'll answer it here. It
> was to be consistent with by using the Debug class.  If a change is made
> to Debug to print elsewhere, what I have now is compatible. Sending it
> to stderr is basically saying that Debug is a useless class and we
> should just print System.err.println() instead.

Good point, although there is already code that does that, see for
example AccessController/AccessControlContext.java.

If we did eventually move Debug to log somewhere else, it would not be
that much effort to fix these few cases. I'm ok either way you decide.

> As for why the stack trace is there, like I said in the comment, if
> there is a situation that should or should not have been blocked, we
> know what path it took.  There are many methods that send data through
> this code and having this debug info allows us to quickly find why it
> happened instead of trying to recreating the situation.  Other
> constraints deny or allow all ops.  This being usage-based, where SHA1
> could be blocked for one usage, but not for another on the same security
> property need more detailed debugging, in my opinion.
>
> About the extra data, I don't believe it's that much more and there
> isn't a way to find out the information that would be needed.  The
> situation of the problem may not be reproducible in our environment
> because the user doesn't want to give up their certs or know their app
> is performing that usage.   If dumpStack() is less data than
> printStackTrace(), I'm fine with switching as long as I get the same info.

It's a little unusual to dump a stack in debug output
(AccessController/AccessControlContext is the only case I know of), but
if it is helpful for future diagnosis, then we should probably keep it.

> It would be nice if there was a verbose option.  We could turn off
> AdaptiveX509Selector that prints every check in byte level detail which
> overflows jtreg's buffer all the time :-)

Please file a followon RFE to do that.

--Sean

>
> Tony
>
> On 03/08/2017 01:42 PM, Xuelei Fan wrote:
>> Looks fine to me except a minor comment.
>>
>> In the update of DisabledAlgorithmConstraints.java, the dumping of stack
>> trace for every checking could increase the debug log size a lot.  There
>> is no verbose option for 'certpath' debug.  What do you think if only
>> dumping the log when the usage is not allowed?
>>
>> Xuelei
>>
>> On 3/8/2017 1:15 PM, Anthony Scarpino wrote:
>>> Hi,
>>>
>>> I need a code review of this small change.. The PKIX path for usage
>>> checking didn't pass the variant to the checkers because of a previous
>>> needed check for plugins.
>>>
>>> http://cr.openjdk.java.net/~ascarpino/8176350/webrev/
>>>
>>> thanks
>>>
>>> Tony
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8176350: Usage constraints don't take effect when using PKIX

Anthony Scarpino
In reply to this post by Anthony Scarpino
On 03/08/2017 01:15 PM, Anthony Scarpino wrote:

> Hi,
>
> I need a code review of this small change.. The PKIX path for usage
> checking didn't pass the variant to the checkers because of a previous
> needed check for plugins.
>
> http://cr.openjdk.java.net/~ascarpino/8176350/webrev/
>
> thanks
>
> Tony

I've updated the webrev in-place with a change that Sean and I talked
about.. This change is a code cleanup related to the bug with PKIX and
plugins, not a behavior change.

Tony

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8176350: Usage constraints don't take effect when using PKIX

Sean Mullan
Looks good. Minor comment in PKIXCertPathValidator.java you can combine
lines 193-194.

--Sean

On 3/9/17 1:29 PM, Anthony Scarpino wrote:

> On 03/08/2017 01:15 PM, Anthony Scarpino wrote:
>> Hi,
>>
>> I need a code review of this small change.. The PKIX path for usage
>> checking didn't pass the variant to the checkers because of a previous
>> needed check for plugins.
>>
>> http://cr.openjdk.java.net/~ascarpino/8176350/webrev/
>>
>> thanks
>>
>> Tony
>
> I've updated the webrev in-place with a change that Sean and I talked
> about.. This change is a code cleanup related to the bug with PKIX and
> plugins, not a behavior change.
>
> Tony
>
Loading...