cloud-init icon indicating copy to clipboard operation
cloud-init copied to clipboard

Add ftp support to NoCloud

Open holmanb opened this issue 1 year ago • 3 comments

Proposed Commit Message

feat: Add support for FTP and FTP over TLS

Use Python's ftplib to connect to and read instance configurations from a
remove server. Like the current http support, nocloud's configuration URI
is defined via kernel command line or dmi product serial.

Notes on FTP and FTP over TLS
=============================

FTP is insecure, use at your own risk.

The FTP over TLS implementation follows library best practices[1][2].
From the ssl documentation regarding create_default_context():

> It will load the system’s trusted CA certificates, enable certificate
> validation and hostname checking, and try to choose reasonably secure
> protocol and cipher settings.

Test Changes
============

tests/i/util.py
---------------

- new helper function verify_clean_boot() to verify cloud-init state
- acquire override_kernel_cmdline() for reuse in other tests
- acquire restart_cloud_init() for reuse in other tests

tests/i/datasources/test_nocloud.py
-----------------------------------

- new test class for FTP/FTPS uses pyftpdlib for server, mkcert for test certs
- 2 test additions for FTP
- 2 test additions for FTPS

Implements via ftplib
=====================

RFC 959 - File Transfer Protocol
RFC 2640 - FTP Internationalization (UTF-8)
RFC 4217 - FTP over TLS

[1] https://docs.python.org/3/library/ftplib.html#ftp-tls-objects
[2] https://docs.python.org/3/library/ssl.html#ssl-security

Additional Context

Test Steps

PLATFORM="lxd_vm" CLOUD_INIT_SOURCE="./cloud-init_all.deb" python -m
pytest -vv --log-cli-level=INFO --durations 10 tests/integration_tests/datasources/test_nocloud.py::TestFTP

Merge type

  • [x] Squash merge using "Proposed Commit Message"
  • [ ] Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

holmanb avatar Feb 01 '24 01:02 holmanb

@holmanb this still WIP?

TheRealFalcon avatar Feb 08 '24 15:02 TheRealFalcon

@holmanb this still WIP?

I'm working on an integration test for it currently, but I think it is ready for review.

holmanb avatar Feb 13 '24 20:02 holmanb

@TheRealFalcon I think I've addressed your comments. I've added integration tests and updated the commit message.

Ready for re-review.

holmanb avatar Feb 24 '24 03:02 holmanb

@TheRealFalcon I believe that I've addressed all of your comments.

I have one more review item left to address, which is the request to include a manually generated cert rather than installing mkcert from source for focal. I won't be able to work on that request until Friday at the earliest, so if you'd like to re-review the non-test code, I don't have any more planned changes there.

holmanb avatar Mar 19 '24 01:03 holmanb

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

github-actions[bot] avatar Apr 05 '24 00:04 github-actions[bot]

rebased and force pushed - addressed remaining comment from @TheRealFalcon and merging

holmanb avatar Apr 15 '24 03:04 holmanb