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

mirror: allow mirroring without explict target #283

Merged
merged 1 commit into from
Dec 6, 2020

Conversation

victorjulien
Copy link
Contributor

Allow omitting the -T option, indicating the target is irrelevant.

The use case is an IDS sensor listening on a dummy interface for the
packets sslsplit produces. The IDS will listen in promisc mode, so the
target is irrelevant.

How I've tested this:

ip link add decrypted type dummy
ip link set decrypted up
<iptables rules to redirect>
sslsplit -j /tmp/sslsplit/  -S logdir/ -k ca.key -c ca.crt -I decrypted https 0.0.0.0 8443
tcpdump -i decrypted

Replaces #282. If code looks good I'll do the doc side next.

Copy link
Collaborator

@sonertari sonertari left a comment

Choose a reason for hiding this comment

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

The first if condition opts->mirrortarget && !opts->mirrorif on line 513 was supposed to remain, the second on line 517 should go (not both). Because we still require mirrorif if mirrortarget is defined. I guess I wasn't clear, sorry.

@sonertari
Copy link
Collaborator

I'd appreciate the documentation too if you have time. Otherwise, I can do the rest. As you wish. Thanks Victor.

@victorjulien
Copy link
Contributor Author

The first if condition opts->mirrortarget && !opts->mirrorif on line 513 was supposed to remain, the second on line 517 should go (not both). Because we still require mirrorif if mirrortarget is defined. I guess I wasn't clear, sorry.

Whoops, good catch. Wanted to go too fast.

@victorjulien
Copy link
Contributor Author

victorjulien commented Dec 6, 2020

I've pushed some updates, fixing the option check and updating the man page and default config. Should there be other updates?

main.c Outdated
fprintf(stderr, "%s: -I depends on -T.\n", argv0);
exit(EXIT_FAILURE);
}
if (opts->mirrortarget && !opts->mirrorif) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, this formatting is off, will fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Allow omitting the -T option, indicating the target is irrelevant.

The use case is an IDS sensor listening on a dummy interface for the
packets sslsplit produces. The IDS will listen in promisc mode, so the
target is irrelevant.
@sonertari
Copy link
Collaborator

I think it's looking good, thanks @victorjulien. I can merge this to the develop branch if you are ok too. @droe can review it there, and it's up to him to merge to the master branch. My guess is that a new release will take a while, so people interested in this change will need to fetch the develop branch until then.

@victorjulien victorjulien marked this pull request as ready for review December 6, 2020 14:56
@victorjulien
Copy link
Contributor Author

Sounds good @sonertari. I've converted it to a non-draft PR.

@sonertari sonertari self-assigned this Dec 6, 2020
@sonertari sonertari merged commit 8cc58cd into droe:develop Dec 6, 2020
@victorjulien
Copy link
Contributor Author

Thanks @sonertari. Would this be something you'd consider for SSLproxy as well? If so I'd be happy to port it and do a PR there as well.

@sonertari
Copy link
Collaborator

Thanks @victorjulien, but it's simple enough, I can do that myself, so you can use your time for more difficult stuff like SSLproxy support for Suricata ;-).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants