Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate latest SFA code #854

Open
ahelsing opened this issue Jul 28, 2015 · 17 comments
Open

Integrate latest SFA code #854

ahelsing opened this issue Jul 28, 2015 · 17 comments
Milestone

Comments

@ahelsing
Copy link
Member

The SFA code has been updated: http://git.planet-lab.org/?p=sfa.git;a=summary

Changes include:

  • credential.get_summary_string() renamed to pretty_cred
  • error messages when a credential fails to verify made a little prettier
  • rename get_subject as pretty_subject
  • rename certificate.cert as certificate.x509

Consider merging in their changes and contributing ours.

@ahelsing ahelsing added this to the 2.10 milestone Jul 28, 2015
@ahelsing
Copy link
Member Author

Last relevant fix appears to be here:
http://git.planet-lab.org/?p=sfa.git;a=commit;h=115294b52c31fd1689d5433f78ac563027640a0e
(from April)

and this:
http://git.planet-lab.org/?p=sfa.git;a=commit;h=5549589f1059734227465220f0d95c96155c1a77
These change some error messages / logs, and change get_subject and pretty_subject

Also, more compact printouts for pretty_cred and pretty_cert (was get_printable_subject or get_summary_tostring)

Also renamed Certificate.cert as Certificate.x509

Some reformatting, some debug printouts when using xmlsec

They are at sfa 3.1-18
Last change was Jun 11

@ahelsing
Copy link
Member Author

The overall changes include a fair bit

  • xrn: Changed separator between URN and ID from - to : and changed the logic in set_authority
  • certificate: Renamed inner cert, changed the format of the pretty print and the method name, and suppress a bunch of debug logs by default.

credential.py:

  • Dropped support for legacy credentials
  • Dropped support for multiple certs in a signature (or never got from me?)
  • Dropped support for ABAC credentials (or never got from me?)
  • renamed / changed the pretty print
  • missing my changes for utf8 and some error checks
  • dropped printing the XML form of the cred
  • Refactored how xmlsec is invoked, to check the process return code and use that to judge success
  • added support for creating a cred from a string or a struct

I'm merging in their fixes without losing the bits I like. But the extent of the changes means this needs to be tested on Windows&Mac, on the clearinghouse, etc.

@ahelsing
Copy link
Member Author

Changes to look for when updating

  • Certificate.cert -> Certificate.x509
  • Certificate.get_printable_subject ->Certificate.pretty_cert`
  • No more Credential.legacy
  • Use Credential.pretty_subject instead of get_subject where appropriate
  • Credential.get_summary_tostring-> Credential.pretty_cred
  • No more Credential.get_lifetime
  • sfatime.DATEFORMAT ->sfatime.SFADATEFORMAT`

These changes impact among other things:

  • delegateSliceCred
  • speaksfor_util when validating speaksfor, among othe rthings
  • cred_util with ABAC Credentials
  • handler_utils._is_user_cert_expired

ahelsing added a commit to ahelsing/geni-tools that referenced this issue Nov 18, 2015
…1-9 of May 2014).

This update includes:
* drop legacy credential support
* invoke xmlxec using subprocess and use the return code to measure success
* rename methods for printing pretty certs and creds
* rename the inner cert in the Certificate

This merge is not a complete replace of what we had with the latest from SFA, as the
latest from SFA does not have ABAC credential support, support multiple certs in the credential signature,
support utf-8 credentials, or include many of our error checks.
@ahelsing
Copy link
Member Author

Tested and verified:

  • gcf-test using APIv2 and v3
  • delegateSliceCred
  • generate and verify a speaks for credential
  • contact the CH
  • Use this on a CH to generate a slice cred, a user cred, a user cert, try to run omni

@ahelsing
Copy link
Member Author

I've now also used speaksfor to talk to a CH running the new gcf, and to talk to a gcf am.

I have not tested the numerous error conditions.

@ahelsing
Copy link
Member Author

I supplied diffs to Thierry. He is integrating my changes back in to SFA.

@ahelsing ahelsing modified the milestones: 2.10, 3.0 Dec 9, 2015
@dmargery
Copy link
Contributor

Hello,

Fed4FIRE has put a new automated test for access to aggregate managers through speaks_for that fails for our BonFIRE delegate based on geni-tools.

It fails when using the master and the develop branch, as well as when using the branch it cut when testing this issue last December (https://github.com/dmargery/geni-tools/tree/tkt854-newsfa)

Looking at the code, and certs, I see that xml:id used in the speaks_for cert is _0, as in
<Signature xmlns="http://www.w3.org/2000/09/xmldsig#" xml:id="Sig__0"> and
<credential xml:id="_0">

but in src/gcf/sfa/trust/credential.py, Signature.decode has
ref_id = sig.getAttribute("xml:id").strip().strip("Sig_")
As strip will remove all leading caracters from string that are part of the string passed as parameter, we have "Sig__0".strip().strip("Sig_") == 0, and not the _0 expected in Certificate.decode to compare credential xml:id and Signature xml:id

Even if this bug is solved (using string. replace), the code of gcf/geni/util/speaksfor_util.py's verify_speaks_for function seems very suspect : It is accessing cred.signature and cred.signature.gid whithout using the dedicated accessors. This goes beyond my debugging abilities but I end up with the following error:

INFO:gcf.am3:Got speaks-for option but not a valid speaks_for with this credential: User URN from cred doesn't match speaking_for URN: None !
= urn:publicid:IDN+wall2.ilabt.iminds.be+user+wvdemeer (cred [ABAC cred: a7154bbb3b779815b635176c19b3530b00eba44d.speaks_for_a7154bbb3b779815
b635176c19b3530b00eba44d<-6e9a4220b5d361c62d2ac61ed1bb456292a19ce7])

I report in this issue because I suspect speaksfor_util to be missing an update after sfa code update.

An ideas ?

David Margery

@dmargery
Copy link
Contributor

The following commit shows how I went one step further in my debugging:
7b11867

@wvdemeer
Copy link

Some extra information: The speaksfor credential used when this problem occurs, is generated by jFed. I have tested jFed speaksfor credentials, and they work on emulab (wall2 with wall2 accounts), and on the geni portal SA/MA (with geni portal accounts).

Are there any other AM's where speaksfor should work? Because it might be interesting to test these with jFed too. Similarly, It could be interesting to test the BonFIRE AM using omni and a speaksfor credential that is not generated by jFed.
Although I'm pretty sure the problem is not with jFed or the speaksfor credential it generates, it never hurts to verify that some more.

@tcmitchell
Copy link
Member

@wvdemeer ExoGENI AMs also handle speaks for credentials so please try there. You can see a list of possibilities at https://portal.geni.net/amstatus.php (search for "Exo").

The GENI clearinghouse SA/MA ought to be using speaks for code from geni-tools (gcf). @dmargery could you try the speaks for parsing from the 2.10 release (without the SFA integration)? Does it work there? Does the relevant parsing code (strip("Sig_")) get introduced in the SAF integration branch?

Thanks!

@dmargery
Copy link
Contributor

@tcmitchell : the offending code using strip to remove the leading Sig_ has been in the code for the last 2 years : https://github.com/GENI-NSF/geni-tools/blob/master/src/gcf/sfa/trust/credential.py#L192

Maybe jFed is the only client generating signatures and credentials with an xml:id starting with an underscore, explaining why that part of the issue has gone undetected until now.

Not running my own CH or SA, I only have one identity. Therefore, generating a speaks_for from an authority that will be seen as valid by my running AM endpoints is not trivial (to check with a speaks_for with no leading _ in xml:id on geni-tools v2.10). So it will be a bit time consuming for me to check if the new sfa code is at fault. I don't believe I can dedicate enough time for that test until February 10, unless you see a shortcut.

@ahelsing
Copy link
Member Author

David,
You are correct - there are a couple bugs here I think:

  • the use of split() in credential.py
  • failing to use the accessors in speaks_for_util - though that one worries me a little; I don't know if there will be side-effects of making that change, so we'd have to test

First: There are multiple places in credential.py that use strip() that will suffer from the same problem. There will be edits needed on lines 192, 197, 199 (only the leading # should be removed). Try making those edits, and see if your problems go away.

Next: You should be able to replicate and test your solution as so:

  1. credential.py line 612 is where the SFA initializes the refid. Set it to _ref0 or _0 to better replicate your problem
  2. Use geni-tools/gen-certs and the GCF CH to generate some credentials - that is how you can run your own CH/SA, and make your AM trust another identity
  3. The speaksfor_utilcan generate a speaks for credential

As for your concerns that speaksfor_util is doing something suspicious: The difference between direct access and via the accessors, is whether decode has been called. If the main problem were failing to use the credential object accessors, then cred.expiration would be None, or cred.signature would be None. (Only when those are None do the accessors call decode().) And if those elements were None, then speaksfor_util would output the error message Credential malformed: missing signature or signer cert. Cred (see line 160). So I don't think that is your problem per se. But it is possible that it is something more subtle - perhaps related to calling decode on the signature object on the credential.

Looking at the error message you got: I suspect it can be traced to needing to do more edits on the use of split as I mentioned above. The error says that the user_urn from the issuer_gid in the signature on the credential is None. If doing more edits as per above doesn't fix this, then the error might happen if something didn't get decoded in the credential or more specifically in the signature on the credential. So you might have better luck if you use a couple of the accessors:

  1. line 152 change cred.expiration to cred.get_expiration()
  2. lines 159 and 161: change cred.signature.gid to cred.signature.get_issuer_gid()
    Those edits should be enough to force a decode on both objects.

But I'd like to see if removing the use of split() fixes this first; I'm nervous of what else the decode may do that we're not expecting and that might impact what speaksfor_util is doing. Put another way: why has this not been a problem so far?

Thanks for finding the issue(s) and reporting what you've found so far. If you manage to do the tests / edits described above, I'm hopeful that will resolve things. Then we'll likely want to open separate issues to track each. I think they will need to be fixed independent of updating to the latest SFA.

@tcmitchell
Copy link
Member

My take is that the strip("Sig_") intends to remove exactly that prefix from the string. Unfortunately, it also removes the next underscore as well from "Sig__0". I think we want to introduce a remove_prefix utility that will remove exactly that prefix. From StackOverlow:

def remove_prefix(text, prefix):
    if text.startswith(prefix):
        return text[len(prefix):]
    return text #or whatever

And then replace strip("Sig_") with remove_prefix(original, "Sig_"). Can you try that out in credential.py and see if it helps?

@wvdemeer
Copy link

wvdemeer commented Feb 1, 2016

@tcmitchell I've tried exogeni as you suggested. I've tested GPO exogeni, and speaks4 works correctly there. I've also added test for cloudlab utah and utah apt, and they also succeed.

But I did find a jFed bug that is relevant! In some of our automated tests, a user credential is sent to the AM instead of a slice credential (so the speaskfor credential is ok, but the other credential isn't). In that case, the AM correctly denies the user access.
This doesn't occur all the time, it seems to occur randomly (which is why I didn't notice it before). So @dmargery: you need to take a closer look at the previous bonfire test failures. I'm afraid this bug might have made debugging harder. I see that some previous tests failed because of this bug, and others failed because of "Request is not authorized by OpenAccess - Reason: Exceeds max storage.". So the speaksfor part might actually already be succeeding here?

I'll fix that bug ASAP.

@wvdemeer
Copy link

wvdemeer commented Feb 1, 2016

Update: The debugging-bug I mentioned has been fixed (it was actually a bug on wall2, not in jFed). So no more occasional false failures during debugging now.

@ahelsing
Copy link
Member Author

ahelsing commented Feb 2, 2016

I have verified that there is a bug in SFA credential parsing, and also verified that the fix @dmargery started and @tcmitchell suggested a full fix for is sufficient in my testing. Please take a look at issue #890. Also take a look at my branch with my proposed fix at https://github.com/ahelsing/geni-tools/tree/tkt890-refid

If that works for you, we can merge that in and also add it to the SFA integration branch.

@ahelsing
Copy link
Member Author

The fix for the issues @dmargery identified and noted on issue #890 are merged on to develop, as well as on to my branch https://github.com/ahelsing/geni-tools/tree/tkt854-newsfa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants