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

T7246: Add base64c validator based on C #4408

Closed
wants to merge 1 commit into from

Conversation

sever-sever
Copy link
Member

@sever-sever sever-sever commented Mar 20, 2025

Change summary

Add validator base64 based on C

$ ./base64c 9JQHZShTNFyslMw+fiK67DE6PiIRKe73aiFk2ejuaw0= --data-length 32
Valid Base64
$
$ ./base64c 9JQHZShTNFyslMw+fiK67DE6PiIRKe73aiFk2ejuaw0=
Valid Base64
$
$ ./base64c foobar
Invalid Base64
$
vyos@r14:~$ /usr/libexec/vyos/validators/base64c 9JQHZShTNFyslMw+fiK67DE6PiIRKe73aiFk2ejuaw0= --data-length 64
Decoded data length mismatch: expected 64, got 32
vyos@r14:~$ 

Compare current and new validator speed:

vyos@r14:~$ time sudo sh -c ' for ((n=0;n<1000;n++)); do /usr/libexec/vyos/validate-value --exec /usr/libexec/vyos/validators/base64 --value "9JQHZShTNFyslMw+fiK67DE6PiIRKe73aiFk2ejuaw0="; done'

real	0m22.709s
user	0m0.004s
sys	0m0.009s
vyos@r14:~$ 
vyos@r14:~$ 
vyos@r14:~$ 
vyos@r14:~$ 
vyos@r14:~$ time sudo sh -c ' for ((n=0;n<1000;n++)); do /usr/libexec/vyos/validate-value --exec /usr/libexec/vyos/validators/base64c --value "9JQHZShTNFyslMw+fiK67DE6PiIRKe73aiFk2ejuaw0="; done'

real	0m3.366s
user	0m0.001s
sys	0m0.007s
vyos@r14:~$ 
vyos@r14:~$ 

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • Other (please describe):

Related Task(s)

Related PR(s)

How to test / Smoketest result

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • I have run the components SMOKETESTS if applicable
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

@sever-sever sever-sever requested a review from a team as a code owner March 20, 2025 21:48
Copy link

github-actions bot commented Mar 20, 2025

👍
No issues in PR Title / Commit Title

Makefile Outdated
@for module in `ls $(VALIDATORS_C_DIR)`
do
# Assuming each module is a C file, adjust if necessary for directories
$(CC) -o src/validators/$$module $(VALIDATORS_C_DIR)/$$module/$$module.c
Copy link
Member

Choose a reason for hiding this comment

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

You should also optimize the code using: -O3

Copy link
Member Author

@sever-sever sever-sever Mar 21, 2025

Choose a reason for hiding this comment

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

You should also optimize the code using: -O3

Done! but the result became worse:

vyos@r14:~$ time sudo sh -c ' for ((n=0;n<1000;n++)); do /usr/libexec/vyos/validate-value --exec /usr/libexec/vyos/validators/base64c --value "9JQHZShTNFyslMw+fiK67DE6PiIRKe73aiFk2ejuaw0="; done'

real	0m4.569s
user	0m0.003s
sys	0m0.005s
vyos@r14:~$ 

Copy link
Member

Choose a reason for hiding this comment

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

Yes. -O3 can really be hit and miss!

Add validator `base64` based on C

```
$ ./base64c 9JQHZShTNFyslMw+fiK67DE6PiIRKe73aiFk2ejuaw0= --data-length 32
Valid Base64
$
$ ./base64c 9JQHZShTNFyslMw+fiK67DE6PiIRKe73aiFk2ejuaw0=
Valid Base64
$
```
Copy link

CI integration 👍 passed!

Details

CI logs

  • CLI Smoketests (no interfaces) 👍 passed
  • CLI Smoketests (interfaces only) 👍 passed
  • Config tests 👍 passed
  • RAID1 tests 👍 passed
  • TPM tests 👍 passed

Copy link
Member

@dmbaturin dmbaturin left a comment

Choose a reason for hiding this comment

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

I'm really against all of the following: adding anything new in C, using a home-grown implementation of Base64 validness check, and especially against home-grown option parsing instead of using getopt.

Here's the deal with geopt. Now the implementation allows only a single length. Quite a few things can come in different length variants: suppose you want to allow keys of lengths 128 or 256 or 512. Now the approach with argc == 4 and the like breaks down because there are no fixed valid argument counts anymore.

Using a Base64 lib and checking the decoded string length isn't that much slower, and we don't need to worry about any possible edge cases.

[dmbaturin@alcor ~/tmp/base64]$ cat base64_ml.ml 
let () =
  let res = Base64.decode @@ Sys.argv.(1) in
  match res with
  | Ok data ->
    if String.length data = 128 then exit 0 else exit 1
  | Error _ -> exit 1
[dmbaturin@alcor ~/tmp/base64]$ ocamlfind ocamlopt -package base64 -linkpkg ./base64_ml.ml -o base64_ml
[dmbaturin@alcor ~/tmp/base64]$ time sh -c ' for ((n=0;n<1000;n++)); do ./base64_ml "9JQHZShTNFyslMw+fiK67DE6PiIRKe73aiFk2ejuaw0="; done'

real	0m1.234s
user	0m0.242s
sys	0m1.012s

If we integrate that in validate-value as a subcommand, that will be a lot faster than calling an external validator.

@sever-sever
Copy link
Member Author

sever-sever commented Mar 21, 2025

If we integrate that in validate-value as a subcommand, that will be a lot faster than calling an external validator.

@dmbaturin Do you think you're going to implement it?

The validator itself faster then OCAML

$ time bash -c 'for ((n=0;n<1000;n++)); do ./base64c "9JQHZShTNFyslMw+fiK67DE6PiIRKe73aiFk2ejuaw0="; done' > /dev/null

real	0m0.668s
user	0m0.279s
sys	0m0.425s
vyos_bld@6b10090e6375:/vyos/work/validators-c/vyos-1x/src/validators$ 

But I like the idea if it could be implemented for the "validate-value as a subcommand"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants