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.

Sign up for GitHub

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

Switch to pyca/cryptography as crypto backend #41

Merged
merged 7 commits into from
Aug 31, 2018
Merged

Switch to pyca/cryptography as crypto backend #41

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b5d88c8
Switch to pyca.cryptography as crypto backend
Aug 7, 2018
e0c8852
new cryptography backend: Fixes after review
Aug 8, 2018
6d6910f
Rename LegacyCertificate to PyCryptoShim
Aug 8, 2018
7439477
Remove reference to rsa_x509_pem in README
Aug 14, 2018
04589cc
Start rework to abstract hash / digest handling into a factory.
Aug 14, 2018
e4e8d83
Replaced home-made cert fingerprinting by pyca's
Aug 14, 2018
a354558
Completely get rid of hashlib and hash-name abstraction
Aug 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 13 additions & 13 deletions src/xmlsec/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ def _get_by_id(t, id_v):

def _alg(elt):
"""
Return the hashlib name of an Algorithm. Hopefully.
Return the xmldsig name of an Algorithm. Hopefully.
:returns: None or string
"""
uri = elt.get('Algorithm', None)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What follows this line is a bit funny. This should just return uri

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one! But I want to emphasize that this piece of code was from @leifj 😛 ... if I read the github blame correctly.

Expand Down Expand Up @@ -327,7 +327,10 @@ def _verify(t, keyspec, sig_path=".//{%s}Signature" % NS['ds'], drop_signature=F
si = sig.find(".//{%s}SignedInfo" % NS['ds'])
logging.debug("Found signedinfo {!s}".format(etree.tostring(si)))
cm_alg = _cm_alg(si)
sig_digest_alg = _sig_digest(si)
try:
sig_digest_alg = _sig_alg(si)
except AttributeError:
raise XMLSigException("Failed to validate {!s} because of unsupported hash format".format(etree.tostring(sig)))

refmap = _process_references(t, sig, verify_mode=True, sig_path=sig_path, drop_signature=drop_signature)
for ref,obj in refmap.items():
Expand All @@ -336,15 +339,15 @@ def _verify(t, keyspec, sig_path=".//{%s}Signature" % NS['ds'], drop_signature=F
sic = _transform(cm_alg, si)
logging.debug("SignedInfo C14N: %s" % sic)
if this_cert.do_digest:
digest = _digest(sic, sig_digest_alg)
digest = _digest(sic, sig_digest_alg.name)
logging.debug("SignedInfo digest: %s" % digest)
b_digest = b64d(digest)
actual = _signed_value(b_digest, this_cert.keysize, True, sig_digest_alg)
actual = _signed_value(b_digest, this_cert.keysize, True, sig_digest_alg.name)
else:
actual = sic

if not this_cert.verify(b64d(sv), actual, sig_digest_alg):
raise XMLSigException("Failed to validate {!s} using sig digest {!s} and cm {!s}".format(etree.tostring(sig),sig_digest_alg,cm_alg))
raise XMLSigException("Failed to validate {!s} using sig digest {!s} and cm {!s}".format(etree.tostring(sig), sig_digest_alg.xmldsig_name, cm_alg))
validated.append(obj)
except XMLSigException, ex:
logging.error(ex)
Expand Down Expand Up @@ -466,7 +469,7 @@ def sign(t, key_spec, cert_spec=None, reference_uri='', insert_index=0, sig_path
si = sig.find(".//{%s}SignedInfo" % NS['ds'])
assert si is not None
cm_alg = _cm_alg(si)
digest_alg = _sig_digest(si)
sig_alg = _sig_alg(si)

_process_references(t, sig, verify_mode=False, sig_path=sig_path)
# XXX create signature reference duplicates/overlaps process references unless a c14 is part of transforms
Expand All @@ -476,14 +479,14 @@ def sign(t, key_spec, cert_spec=None, reference_uri='', insert_index=0, sig_path

# sign hash digest and insert it into the XML
if private.do_digest:
digest = _digest(sic, digest_alg)
digest = _digest(sic, sig_alg.name)
logging.debug("SignedInfo digest: %s" % digest)
b_digest = b64d(digest)
tbs = _signed_value(b_digest, private.keysize, private.do_padding, digest_alg)
tbs = _signed_value(b_digest, private.keysize, private.do_padding, sig_alg.name)
else:
tbs = sic

signed = private.sign(tbs, digest_alg)
signed = private.sign(tbs, sig_alg)
signature = b64e(signed)
logging.debug("SignatureValue: %s" % signature)
sv = sig.find(".//{%s}SignatureValue" % NS['ds'])
Expand Down Expand Up @@ -515,8 +518,5 @@ def _sig_alg(si):
sig_alg = _alg(sm)
if sm is None or sig_alg is None:
raise XMLSigException("No SignatureMethod")
return (sig_alg.split("#"))[1]


def _sig_digest(si):
return (_sig_alg(si).split("-"))[1]
return getattr(constants.Hashes,sig_alg.lower())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.lower is a detail that the caller should not care about. This call belongs to the factory internals.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually kept this -- don't know -- it's internal stuff anyway. Could be implemented in sign_alg_xmldsig_sig_to_internal() but... ... don't know.

55 changes: 49 additions & 6 deletions src/xmlsec/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,59 @@
TRANSFORM_C14N_EXCLUSIVE = 'http://www.w3.org/2001/10/xml-exc-c14n#'
TRANSFORM_C14N_INCLUSIVE = 'http://www.w3.org/TR/2001/REC-xml-c14n-20010315'

ALGORITHM_DIGEST_SHA1 = "http://www.w3.org/2000/09/xmldsig#sha1"
ALGORITHM_SIGNATURE_RSA_SHA1 = "http://www.w3.org/2000/09/xmldsig#rsa-sha1"

ALGORITHM_DIGEST_SHA256 = "http://www.w3.org/2001/04/xmlenc#sha256"
ALGORITHM_SIGNATURE_RSA_SHA256 = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"

ALGORITHM_DIGEST_SHA384 = "http://www.w3.org/2001/04/xmlenc#sha384"
ALGORITHM_SIGNATURE_RSA_SHA384 = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha384"
# All suported crypto primitives and some factories normalization of names as
# pyca/cryptography uses UPPERCASE object-names for hashes.

ALGORITHM_DIGEST_SHA1 = "http://www.w3.org/2000/09/xmldsig#sha1"
ALGORITHM_DIGEST_SHA256 = "http://www.w3.org/2001/04/xmlenc#sha256"
ALGORITHM_DIGEST_SHA384 = "http://www.w3.org/2001/04/xmlenc#sha384"
ALGORITHM_DIGEST_SHA512 = "http://www.w3.org/2001/04/xmlenc#sha512"

ALGORITHM_SIGNATURE_RSA_SHA1 = "http://www.w3.org/2000/09/xmldsig#rsa-sha1"
ALGORITHM_SIGNATURE_RSA_SHA256 = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"
ALGORITHM_SIGNATURE_RSA_SHA384 = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha384"
ALGORITHM_SIGNATURE_RSA_SHA512 = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha512"



class _HashType(object):
def __init__(self, hashlib_name, xmldsig_name, pyca_name=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to be holding on three different names. There should be just one internal name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking into this, I see we need one name for use by xmldsig, one for hashlib and one for pyca. The correct way to implement this is to make it explicit by denoting the respective conversions. We need

  • our own internal name
  • appropriate mappings from each producer
  • appropriate mappings to each consumer/user

The internal name is used everywhere, while conversion takes place on the edges.

sign_alg_internal_to_xmldsig = {
  'sha1': ALGORITHM_SIGNATURE_RSA_SHA1,
  'sha256': ALGORITHM_SIGNATURE_RSA_SHA256,
  'sha384': ALGORITHM_SIGNATURE_RSA_SHA384,
  'sha512': ALGORITHM_SIGNATURE_RSA_SHA512,
}

sign_alg_xmldsig_to_internal = {
  ALGORITHM_SIGNATURE_RSA_SHA1: 'sha1',
  ALGORITHM_SIGNATURE_RSA_SHA256: 'sha256',
  ALGORITHM_SIGNATURE_RSA_SHA384: 'sha384',
  ALGORITHM_SIGNATURE_RSA_SHA512: 'sha512',
}

sign_alg_internal_to_hashlib = {
  'sha1': 'sha1',
  'sha256': 'sha256',
  'sha384': 'sha384',
  'sha512': 'sha512',
}

sign_alg_internal_to_pyca_cryptography = {
  'sha1': 'SHA1',
  'sha256': 'SHA256',
  'sha384': 'SHA384',
  'sha512': 'SHA512',
}

# and the other way around like xmldsig ..

Then, consumers convert to the internal representation and producers to the desired representation.

def _sig_alg(si):
    sm = si.find(".//{%s}SignatureMethod" % NS['ds'])
    sig_alg = _alg(sm)
    if sm is None or sig_alg is None:
        raise XMLSigException("No SignatureMethod")
    return constants.sign_alg_xmldsig_to_internal[sig_alg]

def sign(self, data, hash_alg, pad_alg="PKCS1v15"):
  # [...]
  hash_alg = constants.sign_alg_internal_to_pyca_cryptography[hash_alg]
  hasher = getattr(pyca_hashes, hash_alg)
  padder = getattr(pyca_padding, pad_alg)
  return self.key.sign(data, padder(), hasher())

In the above, the factory is the mapping itself.
If you want to handle errors explicitly you need an actual factory function.

def sign_alg_xmldsig_to_internal(xmldsig_sign_alg):
  try:
    constants.sign_alg_xmldsig_to_internal[xmldsig_sign_alg]
  except KeyError as e:
    raise UnsupportedSignAlg(xmldsig_sign_alg) from e

(which can be generalized to a factory-of-factories by making constants.sign_alg_xmldsig_to_internal a parameter)

PS: the same holds for padding

This now raises another question: Do we need to be managing hashlib names when we are using a full-featured cryptography library? I do not think so. We can get rid of hashlib and use the primitives from pyca/cryptography directly, thus not having to maintain that mapping and also not having to maintain a synchronization of supported hashes between xmldsig, hashlib and pyca/cryptography.

self.name = hashlib_name
self.xmldsig_name = xmldsig_name
if pyca_name is None:
self.pyca_name = self.name.upper()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this (.upper) is correct. Currently and for the algorithms we use, this may hold. But, for the general case, I do not think pyca/cryptography has made any promise to such a rule. They are free to decide how they name the algorithms they implement and how to expose them.

else:
self.pyca_name = pyca_name

class _HashTypes(object):
def __init__(self):
self._HASHES = {
ALGORITHM_SIGNATURE_RSA_SHA1: 'sha1',
ALGORITHM_SIGNATURE_RSA_SHA256: 'sha256',
ALGORITHM_SIGNATURE_RSA_SHA384: 'sha384',
ALGORITHM_SIGNATURE_RSA_SHA512: 'sha512'}

def __getattr__(self, item):
if item in self._HASHES:
return _HashType(self._HASHES[item], item)
else:
raise AttributeError("Hash type '%s' not supported." % item)

class _PaddingType(object):
def __init__(self, name):
self.name = name


class _PaddingTypes(object):
def __init__(self):
self._PADDINGS = ['PKCS1v15']
def __getattr__(self, item):
if item in self._PADDINGS:
return _PaddingType(item)
else:
raise AttributeError("Padding type '%s' not supported." % item)

Hashes = _HashTypes()
Paddings = _PaddingTypes()
24 changes: 9 additions & 15 deletions src/xmlsec/crypto.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ def from_keyspec(keyspec, private=False, signature_element=None):
thread_local.cache = cache
return key


class XMlSecCrypto(object):
def __init__(self, source, do_padding, private, do_digest=True):
# Public attributes
Expand All @@ -84,29 +83,24 @@ def __init__(self, source, do_padding, private, do_digest=True):
self.do_padding = do_padding
self.do_digest = do_digest

def sign(self, data, hash_alg=None):
if hash_alg is None:
hash_alg = "sha1"
def sign(self, data, hash_alg, pad_alg="PKCS1v15"):
if self.is_private:
chosen_hash = getattr(hashes, hash_alg.upper())()
return self.key.sign(data,
padding.PKCS1v15(),
chosen_hash
)
hasher = getattr(hashes, hash_alg.pyca_name)
padder = getattr(padding, pad_alg)
return self.key.sign(data, padder(), hasher())
else:
raise XMLSigException('Signing is only possible with a private key.')

def verify(self, signature, msg, hash_alg=None):
if hash_alg is None:
hash_alg = "sha1"
def verify(self, signature, msg, hash_alg, pad_alg="PKCS1v15"):
if not self.is_private:
try:
chosen_hash = getattr(hashes, hash_alg.upper())()
hasher = getattr(hashes, hash_alg.pyca_name)
padder = getattr(padding, pad_alg)
self.key.public_key().verify(
signature,
msg,
padding.PKCS1v15(),
chosen_hash
padder(),
hasher()
)
except InvalidSignature:
return False
Expand Down