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

Add security considerations text. #106

Merged
merged 5 commits into from
Jun 7, 2023
Merged

Add security considerations text. #106

merged 5 commits into from
Jun 7, 2023

Conversation

dlongley
Copy link
Contributor

@dlongley dlongley commented May 24, 2023

Addresses #70.

This PR removes the issue text in the Security Considerations section and adds text on dataset poisoning. The formal verification text is removed as a non-requirement for this section.


Preview | Diff

@dlongley dlongley requested review from gkellogg and yamdan as code owners May 24, 2023 15:30
@iherman iherman self-requested a review May 25, 2023 06:51
Copy link
Member

@iherman iherman left a comment

Choose a reason for hiding this comment

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

  • I wonder whether it would be useful to suggest some values (based on current experiences) for the number of recursions and/or a reasonable timeout value for the suggested methods. Just to help implementers to start somewhere...
  • (for a future editorial review) If dataset poisoning is the only security issue we refer to, then we may not want to have a separate subsection. It is all right to keep it as is for now, but just keep in mind when we publish the final version.

Copy link
Contributor

@yamdan yamdan left a comment

Choose a reason for hiding this comment

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

Thank you @dlongley!

spec/index.html Outdated Show resolved Hide resolved
Co-authored-by: Dan Yamamoto <dan@iij.ad.jp>
@dlongley
Copy link
Contributor Author

@iherman,

I wonder whether it would be useful to suggest some values (based on current experiences) for the number of recursions and/or a reasonable timeout value for the suggested methods. Just to help implementers to start somewhere...

I thought about that -- but felt like it could vary depending on a number of factors to the point that saying something like the default timeout should be "on the order of a few milliseconds" or the max deep recursions should be "probably 1 for common use" felt wishy-washy.

I think what ecosystem a library was primarily written for (whether it's very general or more specific), whether it runs in a single thread or multiple threads (or has features to allow for other work scheduling), whether its target computing devices have X power or Y, and so on all impact the default values. The defaults are about what the predicted most common use cases will be. Ultimately, it's just about making the canonicalization algorithm run in some "reasonable amount of time" or some "reasonable about of compute" for their biggest user base -- which an implementer needs to figure out.

spec/index.html Outdated Show resolved Hide resolved
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
@philarcher
Copy link
Contributor

Thanks for doing this Dave.

One thing: does canonize mean the same thing as canonicalize in US English? You've used 'canonize' a couple of times which, in my version of English, means the algorithm creates a Saint Dataset (i.e. to canonize is to make someone a saint). See https://www.merriam-webster.com/dictionary/canonize

@TallTed
Copy link
Member

TallTed commented Jun 7, 2023

I believe we should use canonicalize in preference to canonize, throughout.

Copy link
Contributor Author

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

Commenting to use suggestion feature.

spec/index.html Outdated Show resolved Hide resolved
spec/index.html Outdated Show resolved Hide resolved
@dlongley dlongley merged commit e98a348 into main Jun 7, 2023
@dlongley dlongley deleted the security-considerations branch June 7, 2023 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants