1543 emptry crl file fails http tpc pull transfer #1547

ccaffy · 2021-11-02T16:28:30Z

bbockelm · 2021-11-02T16:55:54Z

Does this actually fix #1543 though? In the last comment there, you mention it fails if the CRL file is non-empty and contains no valid CRLs. Shouldn't we use some of the internal XRootD routines to parse the file and see if there's one CRLs present?

abh3 · 2021-11-02T17:03:47Z

Yes, this is something to conisder. However, we should also make sure that malicious injection of an empty/invalid CRL file does not completely eliminate CRL checking for all time. This is a big security loophole. As much as I dislike CRL's they do serve a purpose. So, the solution is not completely simple.

…

On Tue, 2 Nov 2021, Brian P Bockelman wrote: Does this actually fix #1543 though? In the last comment there, you mention it fails if the CRL file is non-empty and contains no valid CRLs. Shouldn't we use some of the internal XRootD routines to parse the file and see if there's one CRLs present? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #1547 (comment)

bbockelm · 2021-11-02T17:19:41Z

However, we should also make sure that malicious injection of an empty/invalid CRL file does not completely eliminate CRL checking for all time

This is a file periodically generated by XRootD in the admin directory. I figure if the attacker owns the admin directory, then it's already game over.

It's also an aggregate of all CRLs available - if the attacker controls all your CRLs, then it's probably also game over.

abh3 · 2021-11-02T17:25:56Z

Agreed, but a periodic message that the system is constantly turning off CRL checking because it keeps encountering an invalid CRL file would be a good idea.

…

On Tue, 2 Nov 2021, Brian P Bockelman wrote: > However, we should also make sure that malicious injection of an empty/invalid CRL file does not completely eliminate CRL checking for all time This is a file periodically generated by XRootD in the admin directory. I figure if the attacker owns the admin directory, then it's already game over. It's also an aggregate of all CRLs available - if the attacker controls all your CRLs, then it's probably also game over. -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #1547 (comment)

bbockelm · 2021-11-02T17:27:15Z

Indeed, that's a fantastic idea! I expect this to be a pretty rare occurrence so a warning message seems warranted.

@ccaffy - does this sound good to you?

ccaffy · 2021-11-03T07:57:18Z

Hi all, thanks for your comments!

To sum things up, you would like to disable the CRL checking and display a warning message if there is at least one wrong CRL encountered during the CRL processing?

That's sounds a good plan to me :)

ccaffy · 2021-11-03T08:02:00Z

Does this actually fix #1543 though? In the last comment there, you mention it fails if the CRL file is non-empty and contains no valid CRLs. Shouldn't we use some of the internal XRootD routines to parse the file and see if there's one CRLs present?

I thought that checking that the aggregated-CRL file is not empty would be sufficient as curl already checks the validity of its content. In my opinion, failing a transfer due to an empty CRL file is not acceptable, but failing a transfer due to a wrong content is acceptable.
Hence why I only added a check on the emptiness of the file ;)

bbockelm · 2021-11-03T13:01:08Z

To sum things up, you would like to disable the CRL checking and display a warning message if there is at least one wrong CRL encountered during the CRL processing?

Not quite -- I'm thinking that if there are no CRLs in the CRL file then we disable CRL checking. Here, I'm willing to define "valid" as simply parseable as a CRL (not looking closely at the contents). Particularly, we should have a routine like this one; instead of looping through all CRLs in a file, return successful as soon as a single one is present.

Actually, that suggests a simpler approach. Since we are writing the CRLs to this file in the first place, why not keep track of the number written and, if there were zero, simply don't invoke the corresponding curl option?

ccaffy · 2021-11-03T15:10:48Z

Actually, that suggests a simpler approach. Since we are writing the CRLs to this file in the first place, why not keep track of the number written and, if there were zero, simply don't invoke the corresponding curl option?

Yes, for me this is fine :) I can implement that 👍

…the concatenated CRL file

…09Crl.cc

ccaffy · 2021-11-10T07:37:03Z

I updated the code yesterday :) Please tell me if this is OK for you so that this fix can be merged :)

xrootd-dev · 2021-11-10T07:40:52Z

It's on my list, you have not been forgotten!

…

On Tue, 9 Nov 2021, ccaffy wrote: I updated the code yesterday :) Please tell me if this is OK for you so that this fix can be merged :) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #1547 (comment) ######################################################################## Use REPLY-ALL to reply to list To unsubscribe from the XROOTD-DEV list, click the following link: https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=XROOTD-DEV&A=1

abh3

A couple of questions and perhaps needed changes before we merge this. Sorry for taking so long.

abh3 · 2021-11-30T20:37:36Z

.gitignore

@@ -58,3 +58,5 @@ xrootd.spec
 dist
 *.egg-info
 bindings/python/VERSION
+cmake-*
+.idea/


Why are these being added? Is it a problem in your own repo?

Hi Andy,

No problem :)

Because I use CLion as IDE with local code deployed on a VM. The cmake-* directories is the equivalent of the build directory.
The .idea directory is created by CLion.
I just do not want these directories to be pushed to the project :)

Now that you said this, it looks like I can put these excludes in my local .git/info/exclude file. I will therefore remove these two lines from the .gitignore file

abh3 · 2021-11-30T22:19:18Z

src/XrdTls/XrdTlsTempCA.cc

@@ -198,11 +209,17 @@ CRLSet::processFile(file_smart_ptr &fp, const std::string &fname)
            return false;
        }
    }
+    if(!m_atLeastOneValidCRLFound)
+        m_atLeastOneValidCRLFound = atLeastOneValidCRLFound;


Doesn't this mean that once a valid CRL is found a subsequent scan that doesn't find one will not be registered as such? Won't that be a problem?

I only modify the variable m_atLeastOneValidCRLFound if this variable is false. So, once a valid CRL is found, the m_atLeastOneValidCRLFound will be set to true and therefore will never be assigned anymore during the XrdTlsTempCA::Maintenance() loop.

That way, we only enable CRL checking if there is at least one valid CRL found during the Maintenance run :)

What about this little issue that doesn't seem quite right.

Yes, but you should consider that the next time the maintenance loop runs there may be no valid CRLs. I know that's unlikely but unlikely things happen. I think you should always update the current state of the maintenance loop otherwise that look does half the j0ob.

If during the next time the maintenance loop runs there is no valid CRL file, do we agree that the CRL checking must be disabled?

I think that if the current CRL file is replaced with a bad CRL file then, yes. It should be disabled. This is dynamic because the maintenance check tries to create a CRL file and if it fails then a) if the previous CRL file is still enforce then we need do nothing, or b) it gets replaced with a null file then we need top disable CRL checking. So, in the end it depends on the state the maintenance run leaves the CRL file (something I have not researched).

abh3 · 2021-12-01T09:42:07Z

Well, not really because it is set false at the start (well initial state). Let me explain the state: a) we find a CRL, the state is set to true. b) second scan and we don't find a CRL. c) What do we do? Two cases: 1) The previous file is kept so it's OK to say use the CRL. 2) The previpus file is replaced with a null file -- not OK. I think you see the conundrum. It really depends on what happens to the compisite CRL file; i.e. is it kept or replaced. So, given that I don't know (as a reviewer) it seems safest to just reflect the current state. Now, you can elucidete what the case is. However it should be made painfully clear in the comments ahead of the test; otherwise we will be revisting this for years to come. Andy

…

On Wed, 1 Dec 2021, ccaffy wrote: @ccaffy commented on this pull request. > @@ -198,11 +209,17 @@ CRLSet::processFile(file_smart_ptr &fp, const std::string &fname) return false; } } + if(!m_atLeastOneValidCRLFound) + m_atLeastOneValidCRLFound = atLeastOneValidCRLFound; Ok, I see what you mean. I just need to reset this boolean to false at the beginning of the Maintenance run 👍 -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #1547 (comment)

ccaffy · 2021-12-01T13:00:48Z

I thought we wanted the composite CRL file to reflect what is in the /etc/grid-security/certificates directory and not trying to see if in the previous run, a CRL that was found is not found anymore or is replaced by a null file in the next run.

Here are the tests I did:

First, best case scenario, there are two valid pem files in the /etc/grid-security/certificates directory:

//Server side
211201 13:42:09 23886 TPC_TempCA: Reloading the list of CAs and CRLs in directory

Composite CRL file:

-----BEGIN X509 CRL-----
[...] CRL1 file content
-----END X509 CRL-----
-----BEGIN X509 CRL-----
[...] CRL2 file content
-----END X509 CRL-----

Now, I empty the CRL2 file content:

root@xrootd-ccaffy-dev01 xrootd-run]# cat /etc/grid-security/certificates/my_crl_file_2.crl
[root@xrootd-ccaffy-dev01 xrootd-run]#

... and after the next run of the Maintenance:

-----BEGIN X509 CRL-----
[...] CRL1 file content
-----END X509 CRL-----

The composite CRL file is re-generated. Containing only the first CRL file as I emptied the second CRL file.

Now I will modify the CRL1 file content to break the CRL correctness of this file:

[root@xrootd-ccaffy-dev01 xrootd-run]# truncate -s 500 /etc/grid-security/certificates/my_crl_file.crl

Then the composite CRL file is re-generated and contains nothing:

[xrootddev@xrootd-ccaffy-dev01 xrootd-run]$ cat /var/spool/xrootd/.xrdtls/crl_file.pem
[xrootddev@xrootd-ccaffy-dev01 xrootd-run]$

A TPC transfer will then have the CRL check disabled:

211201 13:56:48 23872 TPC_TpcHandler: No valid CRL file has been found in the file /var/spool/xrootd/.xrdtls/crl_file.pem. Disabling CRL checking.

Is this the behaviour we expect? Because for me, the composite CRL file should be an image of what is in the /etc/grid-security/certificates directory.

To sum things up, the composite CRL file is completely re-generated at each Maintenance run :)

Sorry for the time spent on this topic, but I really need to understand and I want to be sure that you have the full picture of why I did this implementation like this ;)

…rs/files" This reverts commit 679a87f.

xrootd-dev · 2021-12-02T00:22:14Z

I don't think that would work all that well because there may be a transfer during the maintenance run and then CRLs would not be checked eventhough there still is the previous file. Franklly, I would just get rid of the if statement gaurd. Andy

…

On Wed, 1 Dec 2021, ccaffy wrote: @ccaffy commented on this pull request. > @@ -198,11 +209,17 @@ CRLSet::processFile(file_smart_ptr &fp, const std::string &fname) return false; } } + if(!m_atLeastOneValidCRLFound) + m_atLeastOneValidCRLFound = atLeastOneValidCRLFound; Ok, I see what you mean. I just need to reset this boolean to false at the beginning of the Maintenance run 👍 -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #1547 (comment) ######################################################################## Use REPLY-ALL to reply to list To unsubscribe from the XROOTD-DEV list, click the following link: https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=XROOTD-DEV&A=1

ccaffy · 2021-12-02T07:43:24Z

Alright then, I get rid of the if statement. Thanks for your comments :)

…of the logic so that if at least one valid CRL file is found, the CRL checking is activated

ccaffy added 2 commits November 2, 2021 16:27

[gitignore] gitignore update for ignoring CLion-related folders/files

679a87f

[XrdTpcTPC] An empty CRL file does not fail an HTTP TPC transfer anymore

4d8966b

ccaffy mentioned this pull request Nov 4, 2021

[XrootdHttpTPC] Empty CRL file fails a HTTP TPC pull transfer #1543

Closed

ccaffy added 2 commits November 9, 2021 10:26

[XrdHttpTPC] Disable the CRL checking if no CRL have been written in …

cfeef32

…the concatenated CRL file

[XrdHttpTPC] Removed unnecessary include of sstream in XrdCryptosslX5…

7cf2bc5

…09Crl.cc

abh3 reviewed Nov 30, 2021

View reviewed changes

Revert "[gitignore] gitignore update for ignoring CLion-related folde…

c677815

…rs/files" This reverts commit 679a87f.

[XrdHttpTPC] CRLSet::processFile() - removed the if guard at the end …

fd38d79

…of the logic so that if at least one valid CRL file is found, the CRL checking is activated

abh3 merged commit a384ecb into xrootd:master Dec 3, 2021

ccaffy deleted the 1543-emptry-crl-file-fails-http-tpc-pull-transfer branch March 15, 2022 10:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1543 emptry crl file fails http tpc pull transfer #1547

1543 emptry crl file fails http tpc pull transfer #1547

ccaffy commented Nov 2, 2021

bbockelm commented Nov 2, 2021

abh3 commented Nov 2, 2021 via email

bbockelm commented Nov 2, 2021

abh3 commented Nov 2, 2021 via email

bbockelm commented Nov 2, 2021

ccaffy commented Nov 3, 2021

ccaffy commented Nov 3, 2021

bbockelm commented Nov 3, 2021

ccaffy commented Nov 3, 2021

ccaffy commented Nov 10, 2021

xrootd-dev commented Nov 10, 2021 via email

abh3 left a comment

abh3 Nov 30, 2021

ccaffy Dec 1, 2021

abh3 Nov 30, 2021

ccaffy Dec 1, 2021

abh3 Dec 1, 2021

abh3 Dec 1, 2021

ccaffy Dec 1, 2021

abh3 Dec 1, 2021

abh3 commented Dec 1, 2021 via email

ccaffy commented Dec 1, 2021

xrootd-dev commented Dec 2, 2021 via email

ccaffy commented Dec 2, 2021

1543 emptry crl file fails http tpc pull transfer #1547

1543 emptry crl file fails http tpc pull transfer #1547

Conversation

ccaffy commented Nov 2, 2021

bbockelm commented Nov 2, 2021

abh3 commented Nov 2, 2021 via email

bbockelm commented Nov 2, 2021

abh3 commented Nov 2, 2021 via email

bbockelm commented Nov 2, 2021

ccaffy commented Nov 3, 2021

ccaffy commented Nov 3, 2021

bbockelm commented Nov 3, 2021

ccaffy commented Nov 3, 2021

ccaffy commented Nov 10, 2021

xrootd-dev commented Nov 10, 2021 via email

abh3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abh3 commented Dec 1, 2021 via email

ccaffy commented Dec 1, 2021

xrootd-dev commented Dec 2, 2021 via email

ccaffy commented Dec 2, 2021