refactors AWSSigner; scheme no longer required.

[ianhundere]

resolves #668

Description of the changes being introduced by the pull request:

This refactors AWSSigner to no longer require local_scheme, and instead, gets scheme from AWS' response (e.g. request["SigningAlgorithms"]).

ecdsa:

"SigningAlgorithms": [
    "ECDSA_SHA_256"
]

The caveat is that RSA keys have multiple SigningAlgorithms, so if local_scheme is not provided and the keytype is rsa then rsassa-pss-sha256 is chosen by default as is the case throughout securesystemslib.

rsa:

"SigningAlgorithms": [
    "RSASSA_PKCS1_V1_5_SHA_256",
    "RSASSA_PKCS1_V1_5_SHA_384",
    "RSASSA_PKCS1_V1_5_SHA_512",
    "RSASSA_PSS_SHA_256",
    "RSASSA_PSS_SHA_384",
    "RSASSA_PSS_SHA_512"
]

Also includes updated docs and error handling.

Please verify and check that the pull request fulfills the following requirements:

• The code follows the Code Style Guidelines
• Tests have been added for the bug fix or new feature
• Docs have been added for the bug fix or new feature

cc @jku @lukpueh

[jku]

Thanks. I think this is good to merge: it improves import_() in that it can be used without the other argument.

I'll leave some notes below but I am not requesting changes: I have no access to AWS signer so I can't really tell what is smart and what is not.

So just as a comment: the lookup (from-AWS-response-to-securesystemslib-key-type-and-scheme) still looks more complicated than it probably is

• RSA users can now choose the hash length but ECDSA users for some reason cannot?
• the real import_() argument probably should have been hash_size: Optional[int] = None if that's the only real variable. This would be an API change for AWSSigner so maybe the scheme string still makes sense though
• then the single lookup function would be def _get_keytype_and_scheme(aws_response, hash_size) -> Tuple[str, str] (or def _get_keytype_and_scheme(aws_response, scheme_from_user) -> Tuple[str, str] if the import_() argument is the scheme string)

[lukpueh]

Sorry for commenting late. I am not a big fan of the new key-type specific argument. It breaks the API and feels inconsistent.

I suggest to keep the local_scheme, make it optional, and use it like so:

get key from aws by keyid

if scheme is passed:
  if scheme is not allowed for key
    raise

else:
  assign default scheme for key

I don't know exactly what schemes are supported for what key, but I expect that for any rsa key in the kms you should be able to use any of the securesystemslib schemes, whereas for ecdsa there is only one scheme per key.

A very brief look at the docs seems to support my assumption.

[ianhundere]

@jku @lukpueh thanks for y'all's input, i've gone ahead and renamed rsa-scheme back to local_scheme.

[lukpueh]

fyi @ianhundere, I've tested AWSSigner with Localstack for the first time today and it works like a charm! 🚀 ❤️
repository-service-tuf/repository-service-tuf-worker#452

Thanks for your efforts!

[lukpueh]

Thanks for the updates! There is one small bug in import_ which needs to be fixed before we can merge this. The rest is polishing.

[lukpueh]

Thanks for your persistence! It looks like you are still not consistently checking, if the key supports the chosen scheme according to SigningAlgorithms in the response.

As I said, I'm okay, if we improve that in a follow-up PR.

Please address my inline comment and we can merge this (or let me know, if I should address it).

[ianhundere]

Thanks for the updates! There is one small bug in import_ which needs to be fixed before we can merge this. The rest is polishing.

okay, i think that about does it; thanks for all your feedback. 🙇

[lukpueh]

Thanks for your continued efforts, @ianhundere!