[CI] Remove print stacktrace (due to security concerns)

[ChaiBapchya]

Description

Right now with #17465 if a CI Jenkins build fails, we print the stacktrace using error.printStackTrace() function

I found that
a. According to Jenkins Script Security, It is an insecure function that needs to be approved by administrators in Jenkins -> Manage Jenkins -> In-process Script approval
Without the approval it gives SandBox error

org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException: Scripts not permitted to use method java.lang.Throwable getStackTrace

This was probably the reason why status of few PR runs (despite success/fail on Jenkins) wasn't reported back to Github. Someone from the admin team likely approved it so as to allow the function to not error out.

b. However, the error.printStackTrace() function sends the report to System.err
It doesn't get printed on the console.
To print on console, it has to be passed as a parameter.
Verified it works by adding a failure case in the try (so as to enter the "catch")

try {
    update_github_commit_status('PENDING', 'Job has been enqueued')
    System.out.println(12 / 0)
}

It enters catch and then prints the stack trace for divide by zero exception as expected.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

• Changes are complete (i.e. I finished coding on this PR)
• All changes have test coverage:
• Code is well-documented:
• To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Comments

Read more about Jenkins Script Security : https://plugins.jenkins.io/script-security/

How to handle printStackTrace - https://stackoverflow.com/questions/12095378/difference-between-e-printstacktrace-and-system-out-printlne

[ChaiBapchya]

@mxnet-label-bot add [pr-awaiting-review]

[ChaiBapchya]

@anirudh2290 @access2rohit @marcoabreu
Since you reviewed the previous PR, thought you could take a look at this one.

[marcoabreu]

Have you verified the security implications? Usually, these restrictions are in place for a reason - I'd recommend to first check why the developers considered that function security critical before moving forward.

[ChaiBapchya]

My point about security was a general point. I just made a note of it here.
This was function 'printStackTrace' was already present since #17065
I have only made 1 change (move that logging from std err to std out) - so that it gets print in the console.
Not sure what security consideration should I consider? This change of mine doesn't need any change in the existing Script Security (since it has already added printStackTrace to the approved function list)

It can be verified here - http://jenkins.mxnet-ci.amazon-ml.com/scriptApproval/

[marcoabreu]

The question is which security considerations have been put in place when the function was whitelisted

[ChaiBapchya]

Um it's a common practice to get the stack trace of errors and render them on Jenkins. A cursory glance on google search revealed lots of users doing that. I'm not sure if printing stacktrace poses security risk.
Can you be more specific? Unable to understand exactly what's the contention.

[marcoabreu]

Generally it's discouraged security wise to expose the stack trace of a system to the public since it offers insight into a protected system.

Exceptions are generally logged into a system log which is only accessible to administrators. This is also the case for Jenkins. Printing the stack trace of the Jenkins master to public though is not something that's recommended.

Printing the stack trace within docker is fine, within Jenkins though it's not.

[ChaiBapchya]

@marcoabreu The security concerns are well-founded.

@access2rohit Do you mind explaining here why it was chosen to add printStackTrace to console despite its security concerns? Is it coz
a. Code for CI as well as MXNet codebase in general is open-source & public
b. Errors revealed in stacktrace don't quite give away much info about the protected system.
Did I miss something?

[marcoabreu]

While MXNet might be open source, the Jenkins master is still are security relevant system. It contains credentials, has a user facing website and publishes artifacts.

[aaronmarkham]

@ChaiBapchya This reference in your description goes to a 404.

Publicly displaying your error logs isn't a great idea. Attacker could use it as a sounding board.
Can't we send it to a log file on the system, so on-calls or authorized folks can investigate as needed?

[ChaiBapchya]

Yes It is a 404 due to someone changing the job config. Removed the link from GitHub anyway to prevent any sec issue.

[ChaiBapchya]

Also #17065 introduced print stacktrace which got approved then by same folks.
In the meanwhile, I'm trying to collect what printStackTrace outputs on Jenkins console (if it leaks CI info in any way).

[marcoabreu]

Jenkins already logs to the Jenkins error log which is only accessible by administrators. So I'd say there's no Todo except removing the security except.

[marcoabreu]

Mate, it doesn't matter who did what. You've identified a security issue and it's now on someone to fix it.

The alternative is to have the PMC write an email to [email protected] and have a sev2 filed. I don't understand why we're still discussing this matter.

[ChaiBapchya]

1. I don't mind removing the print statement. Removed it.
2. It was introduced to aid developers to debug faster/easier (of course not at the cost of exposing security). I just wanted to verify if there's any way we can keep this functionality without hampering the security.

Anyway!

[marcoabreu]

I understand and thanks for bringing it up. Sorry for being so hard on this, but it's difficult to compromise on security.

Has the whitelist entry been removed?

[ChaiBapchya]

If I remove whitelist now before this gets merged, it will again block the jenkins build (due to that exception).
Hence to prevent Jenkins build face the exception, I'll remove it from whitelist once this gets merged. What do you reckon?

[marcoabreu]

Sounds good :)

[ChaiBapchya]

@mxnet-label-bot add [pr-awaiting-merge]