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

SubjectHash() returns hash.0 #464

Closed
wants to merge 1 commit into from
Closed

Conversation

ellert
Copy link
Contributor

@ellert ellert commented Feb 14, 2017

XrdCryptoX509::SubjectHash() returns the hash followed by ".0", so make the proper comparison.

@gganis
Copy link
Member

gganis commented Feb 20, 2017

So, this reverts the previous commit be89136, right?

@ellert
Copy link
Contributor Author

ellert commented Feb 20, 2017

No it is not a reversal! Not at all!

That commit is important to make things work, without it things were broken.

However, when i wrote that commit it was not clear to me that the SubjectHash() method did in fact add ".0" to the hash. In the old code, the comparison of the return value from SubjectHash() was sometimes done against the string with the ".0" and sometimes against the string without the ".0" depending on the input data, and it was not obvious when the code was doing the right thing and when it was doing the wrong thing. After the previous commit the comparison was always done against the string without the ".0" (since this was to me the most obvious meaning of the SubjectHash() method name - which was not correct).

The important part of the previous commit was to do the same thing irrespectively whether the input data has the ".0" at the end or not, and it achieved that. And that is what made things work.

The part with the comparison is usually not very crucial since it decides which of the two files should be used for the parent certificate, and normally both are present.

The important parts of the previous commit are the parts that are not touched by this one.

@gganis
Copy link
Member

gganis commented Feb 20, 2017

Hi Mattias,
I see what you mean, there is a difference, in that respect. But adding that flexibility would be a way to hide a problem. I have cross-checked that no server version was supposed to drop the '.0' from the line sent back to the client, so there is a real problem with that old server, running an unmaintained version of XRootD, probably a buffer overflow or similar.
I do not think it is a good idea to change the code to accommodate for a bug in a deprecated version.
I think we should just revert this patch and fix the other problems with the current version that you have spotted and provided possible fixes.

@ellert
Copy link
Contributor Author

ellert commented Feb 21, 2017

I don't think it is a good idea to make some ATLAS data unavailable, even if it is located on an old server. The previous version of the client worked with this server so for many that would be a regression and a reason not to update.

The xrootd server is, as far as I know, not the only software that implements the xrootd server interface. Have you checked what other implementations have been doing? Does the protocol specification specify that the ".0" is mandatory? The client has until know accepted the hash without the ".0", so those implementing the server protocol might not be aware.

@gganis
Copy link
Member

gganis commented Feb 21, 2017

Well, it is not reverting this patch that makes ATLAS data unavailable. Fixing #465 puts makes 4.5.0 and stable-4.6.x behaving exactly the same for what relates to loading-a-CRL-for-an-hash-without-.0 .
The question whether to accept an hash without '.0' should be addressed at a different level in the code.
I am doing that now. This will most likely include reversion of patch be89136 .

@gganis
Copy link
Member

gganis commented Feb 21, 2017

Not needed after reversion of patch be89136 .

@gganis gganis closed this Feb 21, 2017
@ellert ellert deleted the hash-fix branch March 1, 2017 13:30
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

Successfully merging this pull request may close these issues.

None yet

2 participants