[ 
https://issues.apache.org/jira/browse/QPID-7869?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16121515#comment-16121515
 ] 

Alex Rudyy commented on QPID-7869:
----------------------------------

My review comments
* Improve operational logging for trustore with expired certificates. At the 
moment, for the expired certificates the operational log "Certificate expires 
in 0 days" is issued. I would change it to "Certificate is expired". I think we 
need a ceparate operational log for expired certificate.
* NPE for {{SiteSpecificTrustore}}
* documentation is missed for 
{{qpid.truststore.certificateExpiryCheckFrequency}} and 
{{qpid.truststore.certificateExpiryWarnPeriod}}. It is not obvious that their 
values should be expressed in days.
* On other hand, context variables 
{{qpid.truststore.certificateExpiryCheckFrequency}} and 
{{qpid.truststore.certificateExpiryWarnPeriod}} are affectively are duplicates 
of {{qpid.keystore.certificateExpiryCheckFrequency}} and 
{{qpid.keystore.certificateExpiryWarnPeriod}}. IMHO, it make sense to use one 
set of context variables for both keystores and trustores. Maybe, they could be 
named as 'qpid.certificate.expiryCheckFrequency' and 
'qpid.certificate.expiryWarnPeriod'. I do not see any advatage to have 2 sets 
of conetxt variables
* {{QpidPeersOnlyTrustManager}} declares unused field {{_ts}}. It was there 
before the renaming the field, but, taking that this code was changed as part 
of this JIRA, it makes sense to refactor it farther and remove unused field. 
Addtionally, a constructor of {{QpidPeersOnlyTrustManager}} could be refactored 
to pass a list of trust managers, rather than a trustore.
* Methods {{checkCertificateExpiry()}}, {{getCertificateDetails()}} are pretty 
much the same on trust store implementation. It seems they can be moved into 
{{AbstrauctTrustStore}}, if we introduce a protected method 
getCertificatesInternal() to return the list or an array of cached certificates.
* Alternatively, the above can be achieved bay calling an existing method 
{{getCertificates}} from {{checkCertificateExpiry()}}, 
{{getCertificateDetails()}}, though,  we might need to change the 
implementations of {{getCertificates}}. For example, in 
{{NonJavaTrustStoreImpl}} we load certificates on every invocation (I am not 
convinced that it is the right approach). Perhaps, we should reload it only as 
part of certificate expirary check. The same might apply for 
{{FileTrustStoreImpl}}, {{NonJavaTrustStoreImp}}, etc. The fact, that we check 
only cached versions of certificates but use reloaded certificates in trust 
managers would mean that if certificate on disk is updated whilst broker is 
running we will continue to check validity of previous version of certificate. 
The {{SiteSpecificTrustStore}} returns cached certificates from 
{{getCrtificates}}. It seems we have inconcistent implementations of 
{{getCertificates}} accross trust stores.
* {{SiteSpecificTrustStore}} uses its own executor to refresh certificate. It 
seems that broker house keeping executor could be used for certificate refresh. 
IMHO, it make sense to refresh certificate as part of expiry check.
* {{CertificateGridWidget}}
** IMHO, the name of the widget should be changed to 
{{CertificateDetailsWidget}}
** Taking that it is a new widget, it make sense to use dgrid instead of 
deprecated dojo grid.
** {{CertificateGridWidget.html}} ; dom elements contains class from previous 
implementation used to locate nodes (class="removeCertificates"). 
* {Keystore}
** It make sense to expose certificate data as {{CertificateDetails}}
** Potentially, the certificate details can be displayed in UI using 
{{CertificateGridWidget}}

> [Java Broker] Truststore improvements
> -------------------------------------
>
>                 Key: QPID-7869
>                 URL: https://issues.apache.org/jira/browse/QPID-7869
>             Project: Qpid
>          Issue Type: Improvement
>          Components: Java Broker
>            Reporter: Keith Wall
>            Assignee: Keith Wall
>             Fix For: qpid-java-broker-7.0.0
>
>         Attachments: 
> 0001-QPID-7869-Proof-of-concept-only-validate-the-trust-a.patch
>
>
> The current TrustStore API requires some tidy up/improvements to allow an 
> operator to better manage certificate expiry.
> # Currently the details of certificates contained within the store are not 
> exposed in a uniform manner. {#getCertificateDetails}} should be pulled up 
> and implemented by all truststore types.  I suggest we standardise on the 
> form currently used by 
> {{ManagedPeerCertificateTrustStore#getCertificateDetails}} (i.e. the 
> List<CertificateDetails>).  For the {{SiteSpecificTrustStore}} it should 
> return a singleton list.
> # KeyStores currently warn the user certificate are about to expire via 
> operational log messages.  TrustStores should implement the same feature.
> # For SSL client authentication, we should have a 'strict mode' where the 
> {{validFrom}}/{{validTo}} date of the peer certificate is validated before 
> the connection is accepted.    This will help users utilising self signed 
> certificate for client authentication purpose effectively managed certificate 
> expiration.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org

Reply via email to