fuse client, fscrypt, test: Implement and create tests for S_ENCRYPTED in inode i_flags by salieri11 · Pull Request #319 · chrisphoffman/ceph

salieri11 · 2024-11-14T19:44:03Z

This PR adds test for S_ENCRYPTED bit in the i_flags field of Inode.
The test implements 2 quering methods: using FS_IOC_GETFLAGS and STATX_ATTR_ENCRYPTED

Fixes: https://tracker.ceph.com/issues/64129

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows
jenkins test rook e2e

Signed-off-by: Yehuda Sadeh <[email protected]>

Also send encrypted name when linking file Signed-off-by: Yehuda Sadeh <[email protected]>

Signed-off-by: Yehuda Sadeh <[email protected]>

This allows accessing the correct dentry Signed-off-by: Yehuda Sadeh <[email protected]>

Signed-off-by: Yehuda Sadeh <[email protected]>

- file gets its parent fscrypt auth with new nonce - handle response trace: decrypt dname Signed-off-by: Yehuda Sadeh <[email protected]>

Send back info about holes in object. When decrypting object we shouldn't try to decrypt these. Signed-off-by: Yehuda Sadeh <[email protected]>

Signed-off-by: Yehuda Sadeh <[email protected]>

When we're passed inode structure that doesn't have fscrypt_file defined, don't try to override the existing one. Signed-off-by: Yehuda Sadeh <[email protected]>

align the max size to the end of the block Signed-off-by: Yehuda Sadeh <[email protected]>

Signed-off-by: Yehuda Sadeh <[email protected]>

implements that "fscrypt encrypt" functionality Signed-off-by: Yehuda Sadeh <[email protected]>

Signed-off-by: Yehuda Sadeh <[email protected]>

Use encrypted name when building path for mds requests Signed-off-by: Yehuda Sadeh <[email protected]>

When opening the snapdir, use its parent fscrypt ctx Signed-off-by: Yehuda Sadeh <[email protected]>

Encryption params are applied dynamically from the policy. Currently only supporting the two default encryption types. Signed-off-by: Yehuda Sadeh <[email protected]>

Signed-off-by: Yehuda Sadeh <[email protected]>

when opening encrypted files for write, also get the read cap Signed-off-by: Yehuda Sadeh <[email protected]>

round physical size, and update fscrypt_file to reflect requested size Signed-off-by: Yehuda Sadeh <[email protected]>

Fixes: https://tracker.ceph.com/issues/68233 Signed-off-by: Christopher Hoffman <[email protected]>

Convert existing tests to use teuthology framework. Create tests to test N>1 fscrypt clients Fixes: https://tracker.ceph.com/issues/66577 Signed-off-by: Christopher Hoffman <[email protected]>

Provide various fixes in which size used in multi-fuse client tests. Fixes: https://tracker.ceph.com/issues/68431 Signed-off-by: Christopher Hoffman <[email protected]>

Fixes: https://tracker.ceph.com/issues/68798 Fixes: https://tracker.ceph.com/issues/68831 Signed-off-by: Christopher Hoffman <[email protected]>

Signed-off-by: Igor Golikov <[email protected]>

chrisphoffman

I'm having trouble getting this to compile.

error: narrowing conversion of ‘2148034049’ from ‘long unsigned int’ to ‘int’ [-Wnarrowing]
  138 | #define FS_IOC_GETFLAGS _IOR('f', 1, long)

What environment are you testing this on that compiles?

Here's some further info that seems to be related: https://lore.kernel.org/all/[email protected]/T/

src/client/Inode.h

src/client/fuse_ll.cc

src/libcephfs.cc

salieri11 · 2024-11-17T10:41:30Z

I'm having trouble getting this to compile.
error: narrowing conversion of ‘2148034049’ from ‘long unsigned int’ to ‘int’ [-Wnarrowing]
  138 | #define FS_IOC_GETFLAGS _IOR('f', 1, long)
What environment are you testing this on that compiles?

Here's some further info that seems to be related: https://lore.kernel.org/all/[email protected]/T/

I am building both on Ubuntu 24 (arm version), and on vossi04 (i guess its debian), with no errors, using do_cmake.sh
What is your env and how do you build the code?

Signed-off-by: Igor Golikov <[email protected]>

chrisphoffman · 2024-11-20T19:18:54Z

I am building both on Ubuntu 24 (arm version), and on vossi04 (i guess its debian), with no errors, using do_cmake.sh What is your env and how do you build the code?

Fedora and do_cmake as well. I'll try to take a look more at this this week.

Signed-off-by: Igor Golikov <[email protected]>

salieri11 · 2024-11-22T18:51:06Z

I am building both on Ubuntu 24 (arm version), and on vossi04 (i guess its debian), with no errors, using do_cmake.sh What is your env and how do you build the code?

Fedora and do_cmake as well. I'll try to take a look more at this this week.
I changed it to:
#define FS_IOC_GETFLAGS _IOR('f', 1, unsigned long)
This is legit fix to use unsigned

I am building both on Ubuntu 24 (arm version), and on vossi04 (i guess its debian), with no errors, using do_cmake.sh What is your env and how do you build the code?

Fedora and do_cmake as well. I'll try to take a look more at this this week.

I changed it to
#define FS_IOC_GETFLAGS _IOR('f', 1, int32_t)
usually its good enough for these cases, just to use fixed width integer types

src/client/Client.cc

src/client/Client.h

src/test/libcephfs/fscrypt.cc

src/client/Client.h

chrisphoffman · 2024-12-09T18:52:05Z

src/client/Client.cc

+  if (in->is_encrypted()) {
+    stx->stx_attributes |= STATX_ATTR_ENCRYPTED;
+  } else {
+    stx->stx_attributes &= ~STATX_ATTR_ENCRYPTED;


I spent some time looking at client code to see the case for this. Did you see a case where clear will be needed?

edit: reworded for clarity

Well not for now, but in general, I prefer to have clear semantics in such case, without assuming that the flag is clear by default (and not having some garbage that may set the flag to ENCRYPTED)

chrisphoffman · 2024-12-09T18:55:46Z

src/client/Inode.h

+    if (en) {
+      i_flags |= S_ENCRYPTED;
+    } else {
+      i_flags &= (~S_ENCRYPTED);


Is clear explicitly needed? The value will either be default (showing not encrypted) or S_ENCRYPTED. It also shouldn't change should it?

edit: reworded for clarity

Same as above:) For better semantics and defensive code.

I see your reasoning on this. I'm concerned that if there's "garbage" in it, we should let it show instead of hiding the issue.

I'm looking at linux kernel ioctl.c and during
fileattr_fill_flags|fileattr_fill_xflags

it sets value and doesn't worry about clearing.

It makes sense for me now.I think that best will be to assert(false) if is_fscrypt_enabled is false but the S_ENCRYPTED bit is set, before setting the flag.. It wil let us catch the problem, instead of cleaning it.
Same for the previous comment related to. the stx->stx_attributes.

I like that, let's proceed with that change.

Signed-off-by: Igor Golikov <[email protected]>

github-actions · 2024-12-19T10:08:53Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

Signed-off-by: Igor Golikov <[email protected]>

chrisphoffman · 2024-12-19T15:37:00Z

Merged into branch manually due to rebase cleanup work.

See commit here
ceph@6d15f1d

chrisphoffman · 2025-04-25T13:25:38Z

This is a test link, pasting this
https://github.com/chrisphoffman/ceph/commit/d8f6c2cfe248d3af786215eb7e567645c5655b74

without code annotation d8f6c2c

yehudasa added 30 commits October 22, 2024 14:21

fscrypt: fname encryption infrastructure

e771f39

Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: lookup encrypted fname

fecfea2

Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: clean fname encryption/decryption internal api

a716a79

Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: file read path cleanup

7cb7b0a

Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: dentry holds both encrypted and plain names

d056973

Also send encrypted name when linking file Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: use helper for path encryption

6a36de9

Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: encrypt_bl functionality (for file write)

6568c00

Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: use _prepare_req_path() where needed

0af6eb8

This allows accessing the correct dentry Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: implement synchronous read

546b619

Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: implement file write

c6786f7

Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: inherit parent auth, handle trace

c38daf9

- file gets its parent fscrypt auth with new nonce - handle response trace: decrypt dname Signed-off-by: Yehuda Sadeh <[email protected]>

objectcacher: return object holes

53df404

Send back info about holes in object. When decrypting object we shouldn't try to decrypt these. Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: handle holes

889567f

Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: only update the fscrypt_file field if defined

da97a7b

When we're passed inode structure that doesn't have fscrypt_file defined, don't try to override the existing one. Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: update the wanted max size

00e172e

align the max size to the end of the block Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: use effective file size where appropriate

2575680

Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: update effective file size when extending file

7903f1a

Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: create policy ioctl handling

a121a89

implements that "fscrypt encrypt" functionality Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: readlink

f302bfa

Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: symlink

5125971

Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: decode long filenames

a7a3ce7

Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: long file names encoding

f0dbc57

Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: make_nosnap_relative_path() uses enc_name

25e2c24

Use encrypted name when building path for mds requests Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: support snapdir

d6113cc

When opening the snapdir, use its parent fscrypt ctx Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: dynamic encryption params

7725f8c

Encryption params are applied dynamically from the policy. Currently only supporting the two default encryption types. Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: add client api

9bc4749

Signed-off-by: Yehuda Sadeh <[email protected]>

libcephfs: add fscrypt api

5ebf0c9

Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: switch to ldout

ea61af5

Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: conditional caps change

dd3e864

when opening encrypted files for write, also get the read cap Signed-off-by: Yehuda Sadeh <[email protected]>

fscrypt: handle truncate call

4e61a21

round physical size, and update fscrypt_file to reflect requested size Signed-off-by: Yehuda Sadeh <[email protected]>

chrisphoffman and others added 5 commits November 12, 2024 14:49

client: Update fscrypt_file when mds info is newer

003abf1

Fixes: https://tracker.ceph.com/issues/68233 Signed-off-by: Christopher Hoffman <[email protected]>

client: Various fixes to fix multi-fuse client

b4de0f9

Provide various fixes in which size used in multi-fuse client tests. Fixes: https://tracker.ceph.com/issues/68431 Signed-off-by: Christopher Hoffman <[email protected]>

Implement and create tests for S_ENCRYPTED in inode i_flags

ecfe85e

Signed-off-by: Igor Golikov <[email protected]>

github-actions bot added cephfs tests labels Nov 14, 2024

chrisphoffman reviewed Nov 15, 2024

View reviewed changes

src/client/Inode.h Outdated Show resolved Hide resolved

src/client/fuse_ll.cc Outdated Show resolved Hide resolved

src/libcephfs.cc Outdated Show resolved Hide resolved

fixed code review issues

c882a94

Signed-off-by: Igor Golikov <[email protected]>

salieri11 added 2 commits November 21, 2024 15:47

more code review related fixes

322db4b

Signed-off-by: Igor Golikov <[email protected]>

use platform independent type for flag

83b3ffd

Signed-off-by: Igor Golikov <[email protected]>

chrisphoffman reviewed Dec 9, 2024

View reviewed changes

salieri11 added 2 commits December 12, 2024 13:19

code review fix

9409e97

Signed-off-by: Igor Golikov <[email protected]>

fix typo

4fefde7

Signed-off-by: Igor Golikov <[email protected]>

chrisphoffman force-pushed the wip-fscrypt branch 3 times, most recently from 6b620e1 to 33dfe4e Compare December 18, 2024 17:22

github-actions bot added the needs-rebase label Dec 19, 2024

salieri11 added 2 commits December 19, 2024 10:59

fix code review comments

d458e43

Signed-off-by: Igor Golikov <[email protected]>

fi initialization of i_flags, add ceph_assert

e05a788

Signed-off-by: Igor Golikov <[email protected]>

github-actions bot added dashboard pybind build/ops labels Dec 19, 2024

chrisphoffman closed this Dec 19, 2024

Comments

Conversation

salieri11 commented Nov 14, 2024

Contribution Guidelines

Checklist

Uh oh!

chrisphoffman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

salieri11 commented Nov 17, 2024

Uh oh!

chrisphoffman commented Nov 20, 2024

Uh oh!

salieri11 commented Nov 22, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chrisphoffman Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salieri11 Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

chrisphoffman Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salieri11 Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

chrisphoffman Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

salieri11 Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisphoffman Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 19, 2024

Uh oh!

chrisphoffman commented Dec 19, 2024

Uh oh!

chrisphoffman commented Apr 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chrisphoffman Dec 9, 2024 •

edited

Loading

chrisphoffman Dec 9, 2024 •

edited

Loading

salieri11 Dec 18, 2024 •

edited

Loading