-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Add guidance on handling secrets in memory #1257
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very well written and explains the low risk but gives good guidance.
I will add a review this evening. |
On 12/8/2023 at 11:03am ET, @jmanico, you wrote:
and then proceeded to merge this. Exactly 1 hour prior to that, I got received an email notice in my Gmail inbox Now, I don't necessarily disagree with your assessment and in general, you know that I generally am pragmatic and not one to let the perfect become the enemy of the good, but in this case I do wish you would have waited to for me to review this, as I did have some comments that I wanted to make. However, on a broader level, I will say if their are N people who were asked to review a PR, we should at least wait 50% or more of them weigh in or else specific note that they want to decline in commenting / approving. In ESAPI we have its repo configured to require at least 2 approvers for those are are not the code owners (myself, Matt, and Jeremiah) and at least 1 approver if any of the "code owners" submits a PR. It may be prudent to do something similar here. Getting back to my making further comments on this closed PR, given that you merged it and then deleted the branch, I'm not even certain that I can make any additional comments on the MD file. I could of course create another issue on this topic and a 2nd PR, but that seems like an excessive amount of effort for so little updated content. So instead, let me try to describe what I wanted to state and if you agree and @mackowski agree, then perhaps I shall be persuaded to proceed doing that. What I wanted to comment on was this statement:
While this is technically correct, I think it misses the whole threat modeling point that I was trying to make. Anyhow, that is all that I have to see about the PR. I just would have liked the 3 quoted sentences cleared up a bit in regards to stating developers common intuition about memory safety is mostly wrong and that we really need to do threat modeling to help us understand the underlying risks and what to do to address them. |
My apologies. We can. Reverse this out.
|
I hear you, can I see your PR? I’ll take your advice here Kevin even if you wish to remove it.
|
Jim, I am currently busy working with Mark Gamache on updating the ancient "Certificate and Public Key Pinning" OWASP wiki page which is a major rewrite.(And Mark is doing a great job leading.) If I get a few spare cycles, I will give a PR a crack, but would prefer hear what @mackowski thinks about my comments before I proceed with doing that. I already have too many OWASP irons in the fire and with the holidays rapidly approaching, some additional family obligations at all. Also, my apologies if I came off as harsh or critical of you. I know you have the best intentions in mind and I do think this was rather well-written. Thanks for your understanding in this matter. |
If I could represent my love for you Kevin, I’d put it in a variable that was encrypted or protected in some other way so no one could steal it from memory.
That’s how much I care, even when we fight.
❤️
|
Thank you for submitting a Pull Request (PR) to the Cheat Sheet Series.
Please make sure that for your contribution:
[TEXT](URL)
If your PR is related to an issue, please finish your PR text with the following line:
This PR covers issue #1223
Thank you again for your contribution 😃