search

GENI-NSF / geni-tools

Omni, stitcher, GCF sample aggregate manager, and other GENI tools.
Other
11 stars 15 forks source link

Integrate latest SFA code #854

Open ahelsing opened 9 years ago

ahelsing commented 9 years ago

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

Changes include:

Consider merging in their changes and contributing ours.

ahelsing commented 8 years ago

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 commented 8 years ago

The overall changes include a fair bit

credential.py:

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 commented 8 years ago

Changes to look for when updating

These changes impact among other things:

ahelsing commented 8 years ago

Tested and verified:

ahelsing commented 8 years ago

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 commented 8 years ago

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

dmargery commented 8 years ago

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 commented 8 years ago

The following commit shows how I went one step further in my debugging: https://github.com/GENI-NSF/geni-tools/commit/7b118678a7a971ef711fa1bae7d4d2d387f99997

wvdemeer commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

@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 speaksfor 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 commented 8 years ago

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

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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