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

Draft: Scrape FIPS algorithm data #276 #409

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Julik24
Copy link
Collaborator

@Julik24 Julik24 commented May 25, 2024

Added scraping of additional data(type, description, version, capabilities) for each FIPS algorithm

Edit by @adamjanovsky: This closes #276

@Julik24 Julik24 requested a review from adamjanovsky May 25, 2024 15:50
Copy link
Collaborator

@adamjanovsky adamjanovsky left a comment

Choose a reason for hiding this comment

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

Overall, great work (given that this is your first PR into the project 👍). I proposed just minor changes through individual comments. I additionally:

  • Added the issue number to the PR description (so that we know what we're working on)
  • Suggest that we write some minimalistic tests, e.g. downloading a single page and taking a look at how the obtained data from the page looks like. This would belong here

Also, did you get the same algorithms with this approach as you did with the alternative approach that you proposed recently? If not, what is the source of this discrepancy?

By the way, how long does execution of this method take? What's the most time demanding part? The call to FIPSAlgorithmDataset.parse_alg_data_from_html(page) could be parallelized if needed.

src/sec_certs/dataset/fips_algorithm.py Outdated Show resolved Hide resolved
src/sec_certs/dataset/fips_algorithm.py Outdated Show resolved Hide resolved
src/sec_certs/dataset/fips_algorithm.py Outdated Show resolved Hide resolved
src/sec_certs/dataset/fips_algorithm.py Outdated Show resolved Hide resolved
J08nY added a commit that referenced this pull request Jul 4, 2024
Fixes #419.

Also has an impact on #409.
@Julik24 Julik24 requested a review from adamjanovsky September 5, 2024 15:46
@adamjanovsky adamjanovsky marked this pull request as ready for review September 6, 2024 17:59
@adamjanovsky adamjanovsky changed the title Scrape FIPS algorithm data #276 Draft: Scrape FIPS algorithm data #276 Sep 6, 2024
@adamjanovsky adamjanovsky marked this pull request as draft September 6, 2024 18:42
Copy link
Collaborator

@adamjanovsky adamjanovsky left a comment

Choose a reason for hiding this comment

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

Thanks for the update 👍.

There are still two questions from my old comment that are hanging. Could you answer them please?

Also, did you get the same algorithms with this approach as you did with the alternative approach that you proposed recently? If not, what is the source of this discrepancy?

By the way, how long does execution of this method take? What's the most time demanding part? The call to FIPSAlgorithmDataset.parse_alg_data_from_html(page) could be parallelized if needed.

Next time, please contribute by creating branch directly in crocs-muni/sec-certs repository. This way, the automated tests are run against your commits. I manually run the all relevant parts of the pipeline:

  • pre-commit: ❌ does not pass, you have to fix a MyPy error (use np.nan instead of np.NaN in some modules unrelated to your PR, likely due to breaking change in numpy). and apply some Ruff formatting across files.
  • tests: ❌
  • docs: all ok. ✅

The tests failed in FIPS part with

ERROR tests/fips/test_fips_algorithm_dataset.py::test_alg_dset_lookup_dict - TypeError: Dict: {'alg_number': '5331', 'algorithm_type': 'AES', 'vendor': 'Juniper Networks, Inc.', 'implementatio...
ERROR tests/fips/test_fips_algorithm_dataset.py::test_to_pandas - TypeError: Dict: {'alg_number': '5331', 'algorithm_type': 'AES', 'vendor': 'Juniper Networks, Inc.', 'implementatio...

For more info on automated pipelines and how to setup you environment to comply with our standards, please see https://sec-certs.org/docs/contributing.html#quality-assurance.

@J08nY J08nY added enhancement New feature or request fips Related to FIPS 140 certification labels Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fips Related to FIPS 140 certification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scrape FIPS algorithm data
3 participants