DOC Add secure persistence docs #145

BenjaminBossan · 2022-09-19T13:21:01Z

Fixes #139

BenjaminBossan · 2022-09-19T13:47:23Z

@merveenoyan Ready for review.

merveenoyan

Left minor nits, overall it looks good but I previously read the code so I'd like @stevhliu to take a look as well!

README.rst

docs/persistence.rst

merveenoyan · 2022-09-19T15:22:15Z

skops/io/_persist.py

@@ -28,6 +28,29 @@


 def save(obj, file):
+    """Save an object using the skops persistence format.
+
+    Skops aims at providing a secure persistence feature that does not rely on


I think it makes sense to put warnings only to load rather than save, but feel free to ignore if you think it doesn't make sense😅

I know I went overboard with warnings, but I really wanted to make sure users don't miss the fact that we're still early in development and know that it's not secure yet. Hopefully, we can fill these gaps soon and then remove the warnings. Until then, I'd rather leave them in.

Co-authored-by: Merve Noyan <merveenoyan@gmail.com>

stevhliu

Good job and super cool docs!

docs/persistence.rst

stevhliu · 2022-09-21T15:37:57Z

docs/persistence.rst

+   caution as for pickle should be applied: Don't load from sources that you
+   don't trust. In the future, more security will be added.
+
+Skops offers a way to save and load sklearn models without using :mod:`pickle`.


I think this opening would be much stronger if you presented it as an issue with pickle + how skops addresses it. It may not be obvious to a user why they shouldn't use pickle especially if it is so common. Something like:

The pickle module is not secure, but with skops, you can securely save and load sklearn models without using pickle.

docs/persistence.rst

stevhliu · 2022-09-21T15:55:03Z

docs/persistence.rst

+Skops offers a way to save and load sklearn models without using :mod:`pickle`.
+Pickle is the standard serialization format for sklearn and for Python in
+general. It has the big advantage that it can be used for almost all Python code
+but this flexibility also means that it's inherently insecure. As the Python


I think it'd be a little clearer if you also explain why its flexibility means it's insecure. Maybe something like:

but this flexibility also means that it's inherently insecure because [insert reason here].

docs/persistence.rst

stevhliu · 2022-09-21T16:11:17Z

docs/persistence.rst

+code using :func:`eval`. We do, however, have concrete plans on how to mitigate
+this. Please stay updated.
+
+On top of trying to support all of sklearn, we plan on making persistence


Suggested change

On top of trying to support all of sklearn, we plan on making persistence

On top of trying to support persistence in sklearn, we plan on making persistence

stevhliu · 2022-09-21T16:13:04Z

docs/persistence.rst

+work. However, most ``numpy`` and ``scipy`` functions should work. Therefore,
+you can actually save built-in functions like``numpy.sqrt``.
+
+Goals


Hmm, I'd maybe rename this section to just Roadmap and combine it with the section below.

Co-authored-by: Steven Liu <59462357+stevhliu@users.noreply.github.com>

- further explain pickle insecurity - don't use value words - wording - merge sections

BenjaminBossan · 2022-09-22T08:42:32Z

@stevhliu Thanks, your input is really appreciated. I especially tend to use value words, thanks for pointing that out. I addressed all of your points.

adrinjalali

Look good as a first iteration. Thanks @BenjaminBossan

docs/persistence.rst

adrinjalali · 2022-09-26T08:15:52Z

docs/persistence.rst

+code but this flexibility also makes it inherently insecure. This is because
+loading certain types of objects requires the ability to run arbitrary code,
+which can be misused for malicious purposes. For example, an attacker can use it
+to steal secrets from your machine or install a virus. As the Python docs say:


link to the python docs?

I thought about that, but this specific paragraph has no link. However, if you follow the :mod:`pickle` link just above, you will see this paragraph. Is another link to the same place needed?

docs/persistence.rst

skops/io/_persist.py

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

- Wording - Use a "warning" section Also: - Consistent use of "``pickle``" over "pickle".

adrinjalali · 2022-09-26T17:51:46Z

docs/persistence.rst

+attacker can use it to steal secrets from your machine or install a virus. As
+the Python docs say:


I was thinking something like this would be enough?

Suggested change

attacker can use it to steal secrets from your machine or install a virus. As

the Python docs say:

attacker can use it to steal secrets from your machine or install a virus. As

the `Python docs <https://docs.python.org/3/library/pickle.html#module-pickle>`__ say:

This whole paragraph doesn't have any links to pickle.

This results in the exact same link as :mod:`pickle` in the paragraph directly above, but anyway, I added it

BenjaminBossan added 2 commits September 19, 2022 15:19

Add documentation for secure persistence feature

e9d6d92

Add missing rst file

f72c2da

merveenoyan approved these changes Sep 19, 2022

View reviewed changes

BenjaminBossan and others added 3 commits September 19, 2022 17:34

Reviewer comment: fix docs

8a3970f

Co-authored-by: Merve Noyan <merveenoyan@gmail.com>

Reviewer comment: fix docs

d0e98e1

Co-authored-by: Merve Noyan <merveenoyan@gmail.com>

Reviewer comment: fix docstring

c5a9331

Co-authored-by: Merve Noyan <merveenoyan@gmail.com>

stevhliu reviewed Sep 21, 2022

View reviewed changes

BenjaminBossan and others added 8 commits September 22, 2022 10:23

Reviewer comment: fix docs

264d71e

Co-authored-by: Steven Liu <59462357+stevhliu@users.noreply.github.com>

Reviewer comment: fix docs

4ffeb9e

Co-authored-by: Steven Liu <59462357+stevhliu@users.noreply.github.com>

Reviewer comment: fix docs

803edba

Co-authored-by: Steven Liu <59462357+stevhliu@users.noreply.github.com>

Reviewer comment: fix docs

0535c23

Co-authored-by: Steven Liu <59462357+stevhliu@users.noreply.github.com>

Reviewer comment: fix docs

119bd5e

Co-authored-by: Steven Liu <59462357+stevhliu@users.noreply.github.com>

Reviewer comment: fix docs

25dd8df

Co-authored-by: Steven Liu <59462357+stevhliu@users.noreply.github.com>

Reviewer comment: fix docstring

4bb174a

Co-authored-by: Steven Liu <59462357+stevhliu@users.noreply.github.com>

Reviewer comments: Rewrite some docs

867e72c

- further explain pickle insecurity - don't use value words - wording - merge sections

BenjaminBossan mentioned this pull request Sep 22, 2022

Secure persistence: Add documentation #139

Closed

adrinjalali reviewed Sep 26, 2022

View reviewed changes

BenjaminBossan and others added 5 commits September 26, 2022 10:45

Reviewer comment: fix docstring

a130032

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

Reviewer comment: fix docstring

9e86569

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

Reviewer comment: fix docstring

6d2f3c9

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

Reviewer comments:

e00bfa4

- Wording - Use a "warning" section Also: - Consistent use of "``pickle``" over "pickle".

Recommend the use of ".skops" file extension

d8a8d8c

adrinjalali mentioned this pull request Sep 26, 2022

Mark persistence as experimental #153

Closed

adrinjalali reviewed Sep 26, 2022

View reviewed changes

adrinjalali mentioned this pull request Sep 27, 2022

Secure persistence: Add examples #140

Closed

BenjaminBossan and others added 2 commits September 27, 2022 14:13

Reviewer comment: Add link to Python docs

c263ae2

Merge branch 'main' into persistence-docs

23de6bb

adrinjalali changed the title ~~Add secure persistence docs~~ DOC Add secure persistence docs Sep 29, 2022

adrinjalali merged commit 021c8fd into skops-dev:main Sep 29, 2022

BenjaminBossan deleted the persistence-docs branch September 29, 2022 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOC Add secure persistence docs #145

DOC Add secure persistence docs #145

BenjaminBossan commented Sep 19, 2022 •

edited by adrinjalali

Loading

BenjaminBossan commented Sep 19, 2022

merveenoyan left a comment

merveenoyan Sep 19, 2022

BenjaminBossan Sep 19, 2022

stevhliu left a comment

stevhliu Sep 21, 2022

stevhliu Sep 21, 2022

stevhliu Sep 21, 2022

stevhliu Sep 21, 2022

BenjaminBossan commented Sep 22, 2022

adrinjalali left a comment

adrinjalali Sep 26, 2022

BenjaminBossan Sep 26, 2022

adrinjalali Sep 26, 2022

BenjaminBossan Sep 27, 2022

	On top of trying to support all of sklearn, we plan on making persistence
	On top of trying to support persistence in sklearn, we plan on making persistence

		attacker can use it to steal secrets from your machine or install a virus. As
		the Python docs say:

DOC Add secure persistence docs #145

DOC Add secure persistence docs #145

Conversation

BenjaminBossan commented Sep 19, 2022 • edited by adrinjalali Loading

BenjaminBossan commented Sep 19, 2022

merveenoyan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevhliu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenjaminBossan commented Sep 22, 2022

adrinjalali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenjaminBossan commented Sep 19, 2022 •

edited by adrinjalali

Loading