Add assertion param to backed.authenticate and backend.is_authorized

[lucyeun-alation]

PR for #250

In SAML standard doc, section 4.1.4.5 it states The service provider MUST ensure that bearer assertions are not replayed, by maintaining the set of used ID values for the length of time for which the assertion would be considered valid based on the NotOnOrAfter attribute in the <SubjectConfirmationData>..

This is not taken care of by djangosaml2 because djangosaml2 doesn't want to worry about the storage to store the seen assertion ids, but it should provide hooks for the consumer to add the assertion id check. The existing backend.is_authorized can be used, but it does not provide the assertion information, hence adding the assertion as an additional parameter to the is_authorized hook

[peppelinux]

Thank you @lucyeun-alation

I need some more clarifications about:

1. the storage method, adopted to handle this new feature
2. how the lookup would be introduced in relation of the dictionary of all the attributes

regarding the point 2.
wouldn't be better to have just the session-id and not all the attributes?

That's only for a better understanding, I just need to understand in the deep what's your design in this new feature

[lucyeun-alation]

Hi @peppelinux , thanks for the quick review!

We are planning on storing the assertion ids in redis. The is_authorized implementation we are planning on using is following

def is_authorized(self, attributes: dict, attribute_mapping: dict, idp_entityid: str, assertion: object, **kwargs) -> bool:
	
	redis_cache = redis.StrictRedis(db=getattr(settings, 'REDIS_DB_ID'))
    assertion_id = assertion.id

	if redis_cache.get(assertion_id):
		logger.warn("Received SAMLResponse assertion has been already used.")
		return False

	expiration_time = assertion.conditions.not_on_or_after
	time_delta = isoparse(expiration_time) - datetime.now(timezone.utc)
	redis_cache.set(assertion_id, 'True', ex=time_delta)
	return True

We also need the assertion's not_on_or_after info to set the expiry on the cache, so passing the whole assertion instead of the assertion id.

[peppelinux]

Probably a hash of the assertion dict would be good enough, what do you think about that?

And more, the redis TTL I Imagine that Will have the delta equal to (notonafter - now) in seconds. Does It make sense?

Anyway, probably It would be Better to generilize the storage, something not too linked to a specific technology.

We Need a clear and documented approach to handle this new feature, do you agree?

[peppelinux]

Yes, you store assertion.id that's quite opaque and also the timedelta, sounds good

[lucyeun-alation]

Just to make sure I understand, is your suggestion making the assertion param a dictionary, like assertion = {id: '', not_on_or_after: ''} instead of an instance of Assertion object? I'm okay with that. That case we don't need to import the saml2 Assertion object when we need to test the authenticate and is_authorized.

And more, the redis TTL I Imagine that Will have the delta equal to (notonafter - now) in seconds. Does It make sense?
Anyway, probably It would be Better to generilize the storage, something not too linked to a specific technology.
We Need a clear and documented approach to handle this new feature, do you agree?

Redis uses ttl, but other storages may use expiry time, so I think the hook can just provide the not_on_or_after timestamp string from the assertion, and the consumers can calculate the ttl as needed.
Agree on having a clear documentation. Is there anywhere I can add documentation? I think adding the example code I've posted above might be helpful

[peppelinux]

Dict can be serialized if its elements are simple types. So that would be the best choice.

Documentation... Well, we still have a poor readme, we should move to a RTD, but for your proposal a section in the readme would be good enough for now.

Regarding storage, yes, Better a general purpose approach, id+expiration datetime. If not redis, something that prune expired values.

Not at least, this feature should be optionally activated/disactivated. Do you agree or did It sounds too polite?

[lucyeun-alation]

Hi @peppelinux sorry for the delay in the response. I just pushed some changes.

1. Passing an assertion_info dictionary instead of the assertion object, as we discussed
2. Added to the readme with how to use the is_authorized hook to prevent replay attack
3. Updated the unit tests per the change in Point setup.py to the proper README file #1

Not at least, this feature should be optionally activated/disactivated. Do you agree or did It sounds too polite?

I agree that it should be optional. The interface definitely allows the optional configuration, I'm not sure how to make it more obvious in the documentation. Please let me know what you think!

[lucyeun-alation]

SAML doc says
The service provider MUST ensure that bearer assertions are not replayed, by maintaining the set of used ID values for the length of time for which the assertion would be considered valid based on the NotOnOrAfter attribute in the <SubjectConfirmationData>.

The bearer <SubjectConfirmation> element described above MUST contain a <SubjectConfirmationData> element that contains a Recipient attribute containing the service provider's assertion consumer service URL and a NotOnOrAfter attribute that limits the window during which the assertion can be delivered.

[peppelinux]

Hi @lucyeun-alation
you're welcome. Please move that readme section in the new documentaion folder docs/source/contents/setup.rst.
Please update your fork before merge

[lucyeun-alation]

I think I rebased the latest and moved the readme section to docs/source/contents/setup.rst let me know if I missed anything?

[lucyeun-alation]

Thanks @peppelinux for the quick and thorough reviews and @basavakanaparthi-alation for the guidance!

[peppelinux]

you give me the impression that you are two good guys!