italiangrid / voms

The VOMS native service and APIs
https://italiangrid.github.io/voms/
Apache License 2.0
16 stars 19 forks source link

VOMS library silently ignores invalid ACs #51

Closed bbockelm closed 8 years ago

bbockelm commented 8 years ago

If an AC fails to validate, then it is silently ignored by the library:

https://github.com/italiangrid/voms/blob/master/src/api/ccapi/api_util.cc#L338

Particularly, even if all ACs fail to validate and verification is requested, the VOMS_Retrieve routines will return 0 and simply provide no ACs.

It is not clear why this is done, as the commit history is lost due to a SVN merge.

Is this intentional? Is there any verification mode that allows me to see all valid and invalid ACs in the proxy?

Best I can figure is to run VOMS_Retrieve twice and compare the results.

andreaceccanti commented 8 years ago

Is this intentional?

yes. also VOMS Java APIs follow this principle, but allow you to get information on why validation failed for a given AC via a callback or a validation result object.

BTW: the blame shows that this is the behaviour since 2008: https://github.com/italiangrid/voms/blame/master/src/api/ccapi/api_util.cc#L338

Best I can figure is to run VOMS_Retrieve twice and compare the results.

You could do that, but In the end if no ACs are returned you know that no valid ACs were found in the proxy, and if there were ACs that failed the validation you'll get the errors from the OpenSSL error stack.

bbockelm commented 8 years ago

That's not what we observe: if there were ACs that failed validation, you get no errors at all.

To replicate:

This results in a successful return code.

maarten-litmaath commented 8 years ago

Hi all, a VOMS-aware service normally needs a valid AC to determine the roles and groups that the client possesses, and it will typically abort the connection if none could be found. DPM and LFC will fall back to classic DN authN and authZ in such cases, where the VO is then determined from a grid-mapfile variant that is updated through edg-mkgridmap. All in all there appears to be little risk in practice, but I wonder if it would be safer to return a non-zero value in such cases? Maybe non-zero when any error was encountered?

andreaceccanti commented 8 years ago

Tried to replicate Brian's test, and failed. This simple program shows that actually VOMS APIs do report AC signature validation errors:

#include "voms_api.h"
#include <iostream>

int main(int argc, char *argv[]) {
  vomsdata vd;

  if (vd.RetrieveFromProxy(RECURSE_CHAIN)) {
    std::cout << "Verification succeeded!" << std::endl;
    exit (0);
  }
  else {
    std::cout << "Error Message: " << vd.ErrorMessage() << std::endl;
    exit (0);
  }
}
[voms@7889d52f20d5 ~]$ voms-proxy-init -voms test -cert test0.p12
Enter GRID pass phrase for this identity:
Your identity: /C=IT/O=IGI/CN=test0
Creating temporary proxy ......................................... Done
Contacting  localhost:15000 [Whatever] "test" Done
Creating proxy ...................................... Done

Your proxy is valid until Fri Oct  7 23:53:05 2016
[voms@7889d52f20d5 ~]$ voms-proxy-info -all
subject   : /C=IT/O=IGI/CN=test0/CN=proxy
issuer    : /C=IT/O=IGI/CN=test0
identity  : /C=IT/O=IGI/CN=test0
type      : proxy
strength  : 1024 bits
path      : /tmp/x509up_u1234
timeleft  : 11:59:56
key usage : Digital Signature, Key Encipherment
=== VO test extension information ===
VO        : test
subject   : /C=IT/O=IGI/CN=test0
issuer    : /C=IT/O=IGI/CN=dev.local.io
attribute : /test/Role=NULL/Capability=NULL
timeleft  : 11:59:56
uri       : dev.local.io:15000
[voms@7889d52f20d5 ~]$ g++ -g -O0 -I/usr/include/voms/ -lvomsapi  verify.cc
[voms@7889d52f20d5 ~]$ ./a.out
Verification succeeded!
[voms@7889d52f20d5 ~]$ sudo mv /etc/grid-security/vomsdir/test/dev.local.io.lsc /etc/grid-security/vomsdir/
[voms@7889d52f20d5 ~]$ ./a.out
Error Message: Cannot verify AC signature!
msalle commented 8 years ago

Hi, VOMS_Retrieve() indeed returns 0 when no VOMS AC has been returned but following that, you can analyse the reason via the errno passed as last parameter. See e.g. https://ndpfsvn.nikhef.nl/viewvc/mwsec/trunk/lcmaps/src/grid_credential_handling/gsi_handling/lcmaps_voms_attributes.c?annotate=18359#l586 and further. I've just verified with the latest (and previous) VOMS versions, that LCMAPS indeed behaves as expected:

lcmaps_x509_to_voms_fqans(): Warning: Generic verification error for VOMS: AC not valid anymore.. LCMAPS will ignore the VOMS extensions in the mapping sequence.

Furthermore, I agree with Maarten that it is not an issue as we always where of the opinion that the proxy itself may still be used for AuthZ, unless it itself fails verification. So in that case you don't do AuthZ on the VOMS credentials when it is expired, but still can do on the user's DN. In short, I think the current behaviour is fine, although perhaps counterintuitive. Please don't change it!

andreaceccanti commented 8 years ago

Hi Mischa,

In short, I think the current behaviour is fine, although perhaps counterintuitive. Please don't change it!

No way I will change it. It's just a matter of using the API properly. I am embedding also a C api example (written by Vincenzo years ago for a test):

/* ... */
int main(int argc, char *argv[])
{
  struct vomsdata *vd = VOMS_Init(NULL, NULL);
  int error = 0;
  int i = 0;
  BIO *in = NULL;
  char *of = argv[1];
  STACK_OF(X509)* chain = NULL;
  X509 *x = NULL;

  if (vd) {
    in = BIO_new(BIO_s_file());
    if (in) {
      if (BIO_read_filename(in, of) > 0) {
        x = PEM_read_bio_X509(in, NULL, 0, NULL);
        if(!x)
          exit(1);

        chain = load_chain(of);

        if (VOMS_Retrieve(x, chain, RECURSE_CHAIN, vd, &error)) {
          struct voms *voms = VOMS_DefaultData(vd, &error);

          if (voms) {
            char **fqans = voms->fqan;

            while (*fqans) {
              printf("fqan: %s\n", *fqans++);
            }

            exit(0);
          }
        } else {
        printf("VOMS AC valdation failed!\n");
        exit(1);
    }
      }
    }
  }
  exit(1);
}

Closing this issue.