New pips, second try by msalle · Pull Request #18 · argus-authz/argus-pep-server

msalle · 2017-01-12T11:33:36Z

Hi Andrea,
hereby a new pull request.

Adding X509ExtractorPIP and PolicyNamesPIP with corresponding *IniConfigurationParser classes and *Test classes. PolicyNamesPIP makes use of a sub package policynamespip for a caching the info files. This solves argus-authz#15.

Add new PIPs including default config to pepd.ini

@test

junit framework is deprecated in favour of junit tests indicated with @test etc. This requires a newer version of junit.

Add two new policy files which combine the standard IGTF policies, in order to simplify Argus PAP policy matching: WLCG VOs can be matched using policy-aspen-birch-cedar-dogwood while other VOs need to combine with policy-aspen-birch-cedar.

Before trying to get the modification time, test whether the file actually exists.

Introduce intermediate class UpdatingPolicyNamesCache that takes care of the updating of the PolicyNamesCache (formerly PolicyNamesPIPCache). The PIP itself now does not need to keep track of that. Remove the unused and non-trivial possibility of updating the trust dir for a running PIP.

Implement proper read/write locks to prevent memory corruption.

andreaceccanti

Hi Mischa,
I've reviewed the code more deeply (see comments).

In general things look good, but there are things that should be fixed to align it better to Java recommendations for code and other stuff.

The main comment about the updatingCache is about delegating the update responsibility to a background thread. If you don't want to implement that I can do it.

Thanks!

andreaceccanti · 2017-01-13T13:50:13Z

+	<!-- .info files -->
+        <fileSet>
+            <directory>src/main/infofiles</directory>
+            <outputDirectory>/etc/grid-security/certificates</outputDirectory>


I don't like very much the idea that the info files end up in /etc/grid-security/certificates, and that
Argus owns stuff in that directory.

Are these files argus specific? If so, they could live in /etc/argus. If not, they shouldn't be packaged in Argus.

they are indeed Argus specific, in that they don't (and will not) come from IGTF, but they do have the status of policy .info files in the same way as the policy-igtf-classic.info. Having them in /etc/argus will not work therefore, since they would not be found. The reason for having them in the first place, is to prevent us from needing separate permit lines for each individual policy (e.g. classic, mics, slcs and iota) but instead making a single entry with e.g. policy-aspen-birch-cedar-dogwood.
What we could do is put the file itself in /etc/argus/... and make a symlink into the /etc/grid-security/certificates directory.

Since these files are fully argus-specific, then it's up to the PIP to look them up in whatever directory we choose, or am I missing something? I would go for something into /etc/argus/, but also /etc/grid-security/argus/ would be fine for me (following an approach similar to /etc/grid-security/vomsdir).

I've discussed with DavidG and we could try to push them via the EGI policy packages (such as ca-policy-egi-core-1.79-1.noarch.rpm), which are specific to EGI, and actually also produced by David with an EGI hat instead of IGTF hat. The point is that the PIP looks in the 'trustDir' (i.e. normally /etc/grid-security/certificates) for all the info files, and hence that's also where these files should end up, either as file or as symlink. Any other directory does not make sense since they are the same sort of file from the PIP perspective. They are (reasonably) normal info files, only not IGTF.

The point is that the PIP looks in the 'trustDir' (i.e. normally /etc/grid-security/certificates) for all the info file

The point is: are these files meant to be used by other services? If not, they should be packaged in an argus-owned directory, and the PIP modified to look into this directory.
I think the fact that the policy files and certificates must live in the same dir is only a limitation of the current PIP policy lookup implementation, or there's more to it?

First of all, they might be used by other services, in that sense they're not specific to Argus, hence the suggestion to ship them with the EGI policy packages. For most services they are probably not needed, Argus needs them due to the limitation that we can not make logical OR statements in a permit rule, something along the lines of

rule permit { vo="atlas" { ca-policy-names="policy-igtf-classic" OR ca-policy-names="policy-igtf-mics" OR ca-policy-names="policy-igtf-slcs" OR ca-policy-names="policy-igtf-iota" } }

Furthermore, info files have always been in the same directory as the pem files, that includes policy info files such as policy-igtf-classic.info but also the info files for the CAs themselves.

Ok, so it seems we have an agreement that these files should be packaged outside of the Argus PEPD, in a different package, provided by EGI, so please remove them from the assembly file.

done. Same thing for the src/main/assembly/standalone.xml

andreaceccanti · 2017-01-13T13:54:18Z

+	throws PIPProcessingException
+    {
+	long t0=System.nanoTime();
+	boolean pipprocessed=false;


Java favours camelCase, so this should be pipProcessed.

andreaceccanti · 2017-01-13T13:54:30Z

+    {
+	long t0=System.nanoTime();
+	boolean pipprocessed=false;
+	String issuerdn=null;


issuerdn -> issuerDn

andreaceccanti · 2017-01-13T13:55:58Z

+	    for (Attribute attr: attributes) {
+		if (ATTR_X509_ISSUER.equals(attr.getId())) {
+		    // Take first value (it should be singlevalued)
+		    Object tmp = attr.getValues().iterator().next();


tmp is a bad name, probably x509IssuerAttr is a better alternative

andreaceccanti · 2017-01-13T13:56:20Z

+	    }
+
+	    // Look for the issuerdn in the .info files
+	    String[] policynames=new String[0];


policynames -> policyNames

andreaceccanti · 2017-01-13T15:05:00Z

+	read.lock();
+	try {
+	    // Protect against empty cache
+	    if (cache == null)  {


Is there really a case when the cache can be null?

Probably not, perhaps this should be an assertion instead, but in any case I don't like NPEs...

if you don't like NPEs, you should use an Optional for the cache.
I would drop this, since there's really no chance, if the code is written properly, that the cache is null.

andreaceccanti · 2017-01-13T15:06:00Z

+	// First check (with appropriate read lock) whether we need to do
+	// anything. If so, create a new cache.
+	read.lock();
+	try {


I would drop this in favour of a separate thread that just takes the write lock according to a specified period

I agree with the separate thread, but the write lock would also block use of the old cache which is still valid... It's not really an issue, since updating the set won't take that long, but still...

That wouldn't be a problem

andreaceccanti · 2017-01-13T15:06:34Z

+	    // out-of-date (i.e. hasn't been updated in the mean time by another
+	    // thread
+	    if (newCache==null)
+		// This probably never happens: exception will have been thrown


We should drop the code if it never happens

It probably never happens, but as C programmer I really don't like NPEs.

andreaceccanti · 2017-01-13T15:07:03Z

+		// This probably never happens: exception will have been thrown
+		log.warn("New cache is null, continuing to use old one");
+	    else if (cache.getLifeTime() < update_interval)
+		log.info("Other thread appears to have already updated cache");


this can also go away if you use the background updater thread

andreaceccanti · 2017-01-13T15:09:27Z

+		String message= e.getMessage();
+		log.debug("EXPECTED: " + message);
+		log.debug("GOT ("+message.indexOf(element)+"): " + element);
+		assertTrue("PIPProcessingException message does not contain: " + element, message.indexOf(element) >= 0);


instead of these general assertTrue/assertFalse, which produce ugly output difficoult to understand, people use the hamcrest matchers library and the assertThat method, which makes the test and output more readable

Could I ask you to rewrite those? I've mostly copied from how things are currently done in the other PIPs. I agree fully that it's ugly, but then we should change it in all.

I agree we should have consistency, but my objective here is to ensure that new code that enters to the codebase satisfies certain requirements. So please try the migration to hamcrest, and if you have questions/doubts I will be happy to help

I've replaced it in a way similar to https://github.com/italiangrid/voms-api-java/blob/master/src/test/java/org/italiangrid/voms/test/TestVOMSESLineParser.java#L70-L73
so using assertThat with a CoreMatcher. I had to update the junit dependency to 4.11 to get it working.
Is this what you had in mind?

Fixing comments from argus-authz#18 - camelCase - use slf4j placeholders - use String.format() - use {} in all blocks - rethrow exceptions (in certain cases) - log some exceptions before throwing them. - use TimeUnit to convert hours to millisecs - rename some vars for clarity

Fixed one more block missing braces

Make Logger for each PIP private static final.

Use assertThat and a hamcrest matcher instead of a not so readable assertTrue for testing the message of a exception. assertThat needs a slightly newer junit.

The two meta policy info files policy-aspen-birch-cedar-dogwood.info and policy-aspen-birch-cedar.info will be packaged in an external package.

msalle added 9 commits January 12, 2017 12:17

Adding two new PIPs

9899b24

Adding X509ExtractorPIP and PolicyNamesPIP with corresponding *IniConfigurationParser classes and *Test classes. PolicyNamesPIP makes use of a sub package policynamespip for a caching the info files. This solves argus-authz#15.

Add new PIPs to pepd.ini

e8e62af

Add new PIPs including default config to pepd.ini

Update license to match template

f47c425

Increase junit version for new style junit tests

0235546

junit framework is deprecated in favour of junit tests indicated with @test etc. This requires a newer version of junit.

Replace deprecated getValue with getObjectValue

dcd092a

Need to check whether info file exists

fdc241f

Before trying to get the modification time, test whether the file actually exists.

Improve locking using ReentrantReadWriteLock

4d3a5e1

Implement proper read/write locks to prevent memory corruption.

msalle mentioned this pull request Jan 12, 2017

Fixes for issue-15 #16

Merged

andreaceccanti suggested changes Jan 13, 2017

View reviewed changes

msalle added 7 commits January 16, 2017 00:18

Missed one block

92f8935

Fixed one more block missing braces

Make Logger static (and final)

f2e5cd7

Make Logger for each PIP private static final.

Also here make Logger static

42f2436

Use hamcrest for asserting exceptions

79d9bad

Use assertThat and a hamcrest matcher instead of a not so readable assertTrue for testing the message of a exception. assertThat needs a slightly newer junit.

Remove meta policy info files

fe1941d

The two meta policy info files policy-aspen-birch-cedar-dogwood.info and policy-aspen-birch-cedar.info will be packaged in an external package.

Drop test on null cache

3cab79e

Uh oh!

Conversation

msalle commented Jan 12, 2017

Uh oh!

andreaceccanti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreaceccanti Jan 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

andreaceccanti Jan 17, 2017 •

edited

Loading