Skip to content

Comments

Add SNMPKDF implementation#29195

Closed
HelenHZhang wants to merge 4 commits intoopenssl:masterfrom
HelenHZhang:snmpkdf-ws2
Closed

Add SNMPKDF implementation#29195
HelenHZhang wants to merge 4 commits intoopenssl:masterfrom
HelenHZhang:snmpkdf-ws2

Conversation

@HelenHZhang
Copy link
Contributor

@HelenHZhang HelenHZhang commented Nov 21, 2025

In compliance with SP800-135 and RFC7860

Checklist
  • documentation is added or updated
  • tests are added or updated

@loci-dev loci-dev mentioned this pull request Nov 21, 2025
Open
2 tasks
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Nov 23, 2025
@nhorman nhorman requested review from paulidale and slontis December 2, 2025 16:59
paulidale
paulidale reviewed Dec 2, 2025
View reviewed changes
@HelenHZhang HelenHZhang requested a review from paulidale December 5, 2025 01:23
slontis
slontis reviewed Dec 5, 2025
View reviewed changes
slontis
slontis reviewed Dec 5, 2025
View reviewed changes
slontis
slontis reviewed Dec 5, 2025
View reviewed changes
Copy link
Member

@slontis slontis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned previously please use fixup commits when addressing PR comments, and do NOT use merge commits :)

@HelenHZhang HelenHZhang requested a review from slontis December 5, 2025 19:28
slontis
slontis reviewed Dec 8, 2025
View reviewed changes
slontis
slontis previously approved these changes Dec 8, 2025
View reviewed changes
paulidale
paulidale previously approved these changes Dec 9, 2025
View reviewed changes
@paulidale paulidale added approval: done This pull request has the required number of approvals branch: master Applies to master branch triaged: feature The issue/pr requests/adds a feature tests: present The PR has suitable tests present labels Dec 9, 2025
@HelenHZhang HelenHZhang dismissed stale reviews from paulidale and slontis via 0f15213 December 9, 2025 15:37
@slontis
Copy link
Member

slontis commented Dec 9, 2025

My approval still holds once the format changes are done

@paulidale
Copy link
Contributor

paulidale commented Dec 10, 2025

Same here.

@openssl-machine
Copy link
Collaborator

openssl-machine commented Dec 10, 2025

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@paulidale paulidale added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Dec 10, 2025
@paulidale
Copy link
Contributor

paulidale commented Dec 10, 2025

Merged, thanks for the contribution.

@paulidale paulidale closed this Dec 10, 2025
openssl-machine pushed a commit that referenced this pull request Dec 10, 2025
@HelenHZhang @paulidale
Add SNMPKDF implementation
1b03516 
  In compliance with SP800-135 and RFC7860

Reviewed-by: Shane Lontis <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
(Merged from #29195)
@simo5
Copy link
Contributor

simo5 commented Dec 10, 2025

providers/implementations/kdfs/snmpkdf.inc should have been added to .gitignore ...

@HelenHZhang
Copy link
Contributor Author

HelenHZhang commented Dec 10, 2025

I can open another PR to

providers/implementations/kdfs/snmpkdf.inc should have been added to .gitignore ...

I can open another PR to do this. Is this normal way to fix it?

@simo5
Copy link
Contributor

simo5 commented Dec 10, 2025

I can open another PR to

providers/implementations/kdfs/snmpkdf.inc should have been added to .gitignore ...

I can open another PR to do this. Is this normal way to fix it?

I am changing stuff related to FIPS KDFs in #29222 where I noticed this, so I can simply add a quick commit there, no worries.

@simo5
Copy link
Contributor

simo5 commented Dec 10, 2025

Sad that this PR also did not pass style check validations and was pushed anyway ... at this point i think a new PR that fixes style and .gitignore is due.

@HelenHZhang
Copy link
Contributor Author

HelenHZhang commented Dec 10, 2025 •
edited
Loading

Sad that this PR also did not pass style check validations and was pushed anyway ... at this point i think a new PR that fixes style and .gitignore is due.

I started before clang-format conversion. I can open another PR to do clang-format and style fixes.
Do I just snmpkdf.c file?
FYI, a lot of files does not pass clang-format check. Such as fipsprov.c, defltprov.c, which are not related to my changes. But they are in my change set.

@simo5
Copy link
Contributor

simo5 commented Dec 10, 2025

Sad that this PR also did not pass style check validations and was pushed anyway ... at this point i think a new PR that fixes style and .gitignore is due.

I started before clang-format conversion. I can open another PR to do clang-format and style fixes. Do I just snmpkdf.c file? FYI, a lot of files does not pass clang-format check. Such as fipsprov.c, defltprov.c, which are not related to my changes. But they are in my change set.

It is already being fixed here: #29369

cxx194832 pushed a commit to cxx194832/openssl that referenced this pull request Dec 12, 2025
@HelenHZhang @cxx194832
Add SNMPKDF implementation
18ceca7 
  In compliance with SP800-135 and RFC7860

Reviewed-by: Shane Lontis <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
(Merged from openssl#29195)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@paulidale paulidale Awaiting requested review from paulidale

@slontis slontis Awaiting requested review from slontis

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch severity: fips change The pull request changes FIPS provider sources tests: present The PR has suitable tests present triaged: feature The issue/pr requests/adds a feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@HelenHZhang @slontis @paulidale @openssl-machine @simo5