Support using env vars for macOS notarization even if a macOS config exists

[kevinaboos]

Thanks for this fantastic crate! We're starting to use it for packaging up some Project Robius applications.

This PR fixes a minor mistake where the notarization credentials specified via environment variables are not used if a macos-specific config ([package.metadata.packager.macos]) has been specified in the Cargo.toml, but that config section doesn't include the notification_credentials field.

Currently there is another related bug in the schema parsing that doesn't actually allow you to provide the notarization_credentials in the package metadata of Cargo.toml, so this change is actually the only way to currently use the notarization features on macOS, as far as I can tell.

More specifically, when I attempt to specify the notarization_credentials field in [package.metadata.packager.macos], I get an error about that field not existing in the schema. I'm not personally sure how to fix that, though.
However, even when that has been fixed, I still think it's beneficial to be able to specify secrets like this via env vars on the command line, instead of being required to place it in the Cargo.toml file, for security purposes.

[kevinaboos]

Thanks for approving the CI checks. I took a look at the clippy logs and the failures do not appear to be related to this PR's small changeset.

[amr-crabnebula]

Currently there is another related bug in the schema parsing that doesn't actually allow you to provide the notarization_credentials in the package metadata of Cargo.toml, so this change is actually the only way to currently use the notarization features on macOS, as far as I can tell.

More specifically, when I attempt to specify the notarization_credentials field in [package.metadata.packager.macos], I get an error about that field not existing in the schema. I'm not personally sure how to fix that, though. However, even when that has been fixed, I still think it's beneficial to be able to specify secrets like this via env vars on the command line, instead of being required to place it in the Cargo.toml file, for security purposes.

This is intentional for security, as passwords shouldn't be specified in command line and notarization_credentials field can only be set programmatically when using cargo-packager as a library not a CLI.

[amr-crabnebula]

Thanks for the PR, just need a change file in .changes directory with the following content

---
"cargo-packager": "patch"
"@crabnebula/packager": "patch"
---

On macOS, fix notarization skipping needed environment variables when macos specific config has been specified in the config file.

and also a little nit-pick I left below

[kevinaboos]

This is intentional for security, as passwords shouldn't be specified in command line and notarization_credentials field can only be set programmatically when using cargo-packager as a library not a CLI.

Ah I see, thanks for clarifying. Makes sense. As a new user of this crate, I figured it was just an accidental oversight or a mismatch between the schema and Rust-level struct defs.

Given that this is intentional, I think it'd be helpful to specify that design choice via a notice/warning in the doc comments of that notarization_credentials field. If you're open to that, I'm happy to do so (as part of a separate PR), and if you can link me to a list of all the fields that have that restriction, I can even apply that change to all of the "restricted" fields in other parts of Config.

[kevinaboos]

Also, while we're on this topic, I do have a couple of other questions about that design philosophy, i.e., what is and is not exposed via .toml package metadata fields vs. CLI args vs. env vars vs. programmatic access only.

• The signing_identity on macOS is also somewhat of a sensitive field that ideally wouldn't be committed to a public git repo, but currently, it can only be specified in the .toml package metadata (unless I'm missing another way to do it). Seems like it'd be a good idea to support providing that on the command line, either via env vars or traditional CLI args.
  • (I'm not actually 100% sure if this qualifies as "sensitive" data, but I have been instructed not to share/expose it to others.)
• In general, is there something inherently bad about accepting secrets via CLI args as opposed to env vars? In multi-platform setups, it's much simpler to use and define CLI args (which can always use the same structure) than it is to define env vars, which often differ across host OSes and may be cleared by various runtime environments. Not a big deal either way -- I'm just asking for future reference such that I don't waste your time by creating a PR that supports providing secrets via CLI args, if that is not desired.
• If certain fields can only be set programmatically, that implies that we must effectively create another meta-level packaging tool that builds atop cargo-packager. I had been trying to avoid doing this, since I feel it'd lead to more ecosystem fragmentation and reduce the likelihood of us contributing features back into cargo-packager. Either way, our higher-level tool would have to accept similar parameters via files, CLI args, env vars, etc, so someone at some level of the tooling stack would be facing the exact same problem.
  • My thinking is that we might as well solve that here, such that other tools that depend on cargo-packager don't accidentally mess this up and accidentally expose secrets.
  • I do understand if you don't want to expand the scope of cargo-pacakger to support this, though.
• Perhaps a good solution would be to accept a path to a file that defines certain security-sensitive fields (notarization creds, signing details, etc)? My rationale here is that by having all those details provided in a separate file, we can easily exclude it from git commits via .gitignore and/or a CI check.

[kevinaboos]

Finished all requested changes. Thanks for the feedback!

[amr-crabnebula]

Given that this is intentional, I think it'd be helpful to specify that design choice via a notice/warning in the doc comments of that notarization_credentials field. If you're open to that, I'm happy to do so (as part of a separate PR), and if you can link me to a list of all the fields that have that restriction, I can even apply that change to all of the "restricted" fields in other parts of Config.

Yeah, that would be nice, there is not many fields that are ignored when parsing config file but those can be easily found which will have #[serde(skip] attribute on them

[amr-crabnebula]

• The signing_identity on macOS is also somewhat of a sensitive field that ideally wouldn't be committed to a public git repo, but currently, it can only be specified in the .toml package metadata (unless I'm missing another way to do it). Seems like it'd be a good idea to support providing that on the command line, either via env vars or traditional CLI args.

  • (I'm not actually 100% sure if this qualifies as "sensitive" data, but I have been instructed not to share/expose it to others.)

I am not a macOS developer, so can't really say whether it is sensitive or not, so I'd rather leave this to @lucasfernog-crabnebula to answer, but having it as a CLI arg seems fine to me.

[amr-crabnebula]

• In general, is there something inherently bad about accepting secrets via CLI args as opposed to env vars? In multi-platform setups, it's much simpler to use and define CLI args (which can always use the same structure) than it is to define env vars, which often differ across host OSes and may be cleared by various runtime environments. Not a big deal either way -- I'm just asking for future reference such that I don't waste your time by creating a PR that supports providing secrets via CLI args, if that is not desired.

There is nothing bad about using CLI args or env vars, both should be acceptable. I usually see secrets stored in env vars and then passed to CLI args, for example cli --arg1 $SECRET_ARG --arg2 $SECRET_ARG2 so I skipped the CLI options, but we can add these if desired. This could be a positive refactor as it would cleanup some manual std::env::vars calls and rely on env feature.

[amr-crabnebula]

• If certain fields can only be set programmatically, that implies that we must effectively create another meta-level packaging tool that builds atop cargo-packager. I had been trying to avoid doing this, since I feel it'd lead to more ecosystem fragmentation and reduce the likelihood of us contributing features back into cargo-packager. Either way, our higher-level tool would have to accept similar parameters via files, CLI args, env vars, etc, so someone at some level of the tooling stack would be facing the exact same problem.
  • My thinking is that we might as well solve that here, such that other tools that depend on cargo-packager don't accidentally mess this up and accidentally expose secrets.
  • I do understand if you don't want to expand the scope of cargo-pacakger to support this, though.

The reasons why I wouldn't want these be explicitly managed here is to allow flexibility for a higher tool atop of cargo-packager, namely tauri-bundler or tauri-cli which could use different environment variables names for these secrets.

I wouldn't say this would fragment the ecosystem at all, since it is the CLI frontend that would need to be implemented atop of cargo-packager which only does:

1. find relevant config files and deserialize them into Config or just construct Config struct programmatically
2. read env vars if needed
3. read cli args if needed
4. pass config to cargo-packager library

So, 1 to 3, are pretty standard things for any wrapper CLI to do, and I don't see a way around not implementing these in the wrapper other than using a prebuilt binary of our CLI.

[kevinaboos]

those can be easily found which will have #[serde(skip] attribute on them

Excellent, thanks for the tip.

[kevinaboos]

The reasons why I wouldn't want these be explicitly managed here is to allow flexibility for a higher tool atop of cargo-packager, namely tauri-bundler or tauri-cli which could use different environment variables names for these secrets.

Sounds good to me, thanks for sharing your view here.

I wouldn't say this would fragment the ecosystem at all, since it is the CLI frontend that would need to be implemented atop of cargo-packager which only does:

By this, I merely meant that we (Project Robius) are trying to avoid creating tools that duplicate functionality that already exists in the ecosystem. I've also discussed this goal at length with the Linebender project, though not relevant to this packaging concern specifically. Either way, it's okay with me --- we'll continue on the path to creating a frontend wrapper around cargo-packager, and I can then periodically submit PRs here for features that I consider worthy of including in cargo-packager itself.

[amr-crabnebula]

Thanks for your contributions

[kevinaboos]

Thanks, and thank you for all the additional information about this crate!

[kevinaboos]

I am not a macOS developer, so can't really say whether it is sensitive or not, so I'd rather leave this to @lucasfernog-crabnebula to answer, but having it as a CLI arg seems fine to me.

Turns out that whether the signing_identity is considered a secret is a bit of a controversial opinion. Prior to 2014, there was an Apple bug that caused a lot of people to keep their signing identity secret, but now there is technically no reason not to do it. Given that I've been asked to keep it private, I think I'll still proceed with adding a feature that accepts the signing_identity via a command-line arg or env var.